Skip to content

[OPENJPA-3016] Stop two CompUser fixtures sharing a table, and release the EntityManager when a criteria setUp fails - #218

Open
rzo1 wants to merge 2 commits into
masterfrom
OPENJPA-3016
Open

rzo1 wants to merge 2 commits into
masterfrom
OPENJPA-3016

Conversation

@rzo1

@rzo1 rzo1 commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

The first scheduled run of the new Databases workflow was red on MariaDB 11.8: the criteria package failed in bulk and the connection pool was then exhausted, TestMultiselect spending 170s purely on Timeout waiting for idle object. Both problems are in the test suite, on master, and independent of each other — no production code changes here. Two commits so they can be reviewed apart.

1. Two unrelated CompUser hierarchies defaulted to the same table. o.a.o.persistence.criteria.CompUser and o.a.o.persistence.common.apps.CompUser both map to CompUser with incompatible columns, so whichever package runs second makes SynchronizeMappings=buildSchema(SchemaAction=drop,add) rewrite the table on every EntityManagerFactory — and CriteriaTest builds one per test method. MariaDB's general query log shows 105 ALTER TABLE CompUser DROP COLUMN in a single test class. MariaDB keeps dropped columns in the clustered-index record format, so they keep counting towards the 8126-byte row limit until ERROR 1118 Row size too large. A pure-SQL replay of those 14 statements fails at cycle 14 on mariadb:11.8 and 11.4, while mysql:8.0 survives 25 cycles — which is exactly why the MySQL job was green on the same suite. Fix: an explicit @Table(name = "CMN_COMPUSER") on the common/apps fixture. The entity name is untouched, so both the jpql tests' JPQL and the criteria tests' expected SQL stay valid.

Why it never showed locally: surefire's default runOrder=filesystem. On the Linux runner jpql runs before criteria; on macOS the reverse. Measured on mariadb:11.8 in CI order, on a fresh database — the jpql phase is 123 tests and green either way, the criteria phase goes from 540 tests / 133 failures / 183 errors (513 of them Row size too large) to 544 / 0 / 0.

2. A failure in setUp left a connection in the pool for good. junit.framework.TestCase.runBare() calls setUp() outside its try/finally, so a failure there skips tearDown() entirely. CriteriaTest.setUp() opens an EntityManager, and subclasses begin a transaction in their own setUp(), which makes the store manager retain a JDBC connection; CriteriaTest.emf is static and shared by all 24 criteria classes, so each failure leaked one connection out of a pool sized MaxTotal=10. About a dozen failures emptied it, and everything downstream then failed with a pool timeout instead of its own problem. AbstractCriteriaTestCase.runBare() now runs setUp() inside the guarded block and calls tearDown() exactly once either way (what JUnit 4 does for @Before/@After), and CriteriaTest.tearDown() is made null- and exception-safe, since an exception out of em.close() previously skipped emf.close() and leaked the whole pool. Raising MaxTotal or the wait timeout would only postpone the symptom, so the profiles are left alone.

Both new tests fail without their change (verified by reverting each in a throwaway worktree). The MariaDB row-format behaviour is user-facing on its own — an application using buildSchema(drop,add) or mappingtool -action refresh hits the same rotation — so it is documented in supported_databases.xml, with the remedy.

Verified: criteria + jpql packages 667/0/0 on both Derby and H2; on mariadb:11.8 as above.

The third item in the ticket, TestPessimisticLocks on PostgreSQL, is not in this PR. A change for it was written and then dropped: the test passes 6/6 on postgres:11 on repeated runs here, the failure could not be reproduced, and the justification quoted SQL (... FOR UPDATE) that does not appear in the captured statements at all. It is order-dependent in principle — the ranged query carries no ORDER BY, so setFirstResult(1) skips whichever row the server returns first — but that belongs with OPENJPA-3000, which already tracks lock-manager flakes on PostgreSQL and Oracle, and needs a reproduction before a fix.

rzo1 added 2 commits October 13, 2026 14:53
Two unrelated entity hierarchies in the test suite were rooted at a class
called CompUser - org.apache.openjpa.persistence.criteria.CompUser and
org.apache.openjpa.persistence.common.apps.CompUser - and both defaulted to
a table of that name, with incompatible columns: userid is NOT NULL in one
and nullable in the other, ADD_ID is BIGINT in one and INTEGER in the other,
and only the common/apps one has nameAsLob.

Whichever package runs second therefore makes
SynchronizeMappings=buildSchema(SchemaAction='drop,add') rewrite the table:
seven ALTER TABLE ... DROP COLUMN followed by seven ALTER TABLE ... ADD, on
every EntityManagerFactory it creates. CriteriaTest creates and closes one
factory per test method, so a single criteria class rotates every column of
CompUser dozens of times.

MariaDB executes such an ADD/DROP COLUMN instantly and keeps the dropped
columns in the clustered index record format, counting them towards the
8126 byte maximum row size. After about a dozen rotations the next statement
is rejected:

  ALTER TABLE CompUser ADD DTYPE VARCHAR(255)
  ERROR 1118 (42000): Row size too large. The maximum row size for the used
  table type, not counting BLOBs, is 8126.

This is the server's behaviour, not a defect in OpenJPA or in its MariaDB
dictionary: replaying the same fourteen statements by hand against an
otherwise untouched InnoDB table fails on the fourteenth cycle, while
information_schema still reports the eight visible columns. MySQL 8.0 does
not accumulate the metadata, which is why the scheduled Databases run was
red only for MariaDB.

Nothing about the collision is MariaDB specific, though, so the fixture is
separated rather than the dictionary taught to tolerate it: an explicit
@table(name = "CMN_COMPUSER") keeps the two hierarchies apart and makes the
suite independent of the order in which surefire happens to list the test
classes - which is what hid this locally, where the criteria package runs
before the jpql one.

The entity name is unchanged, so the JPQL in the jpql tests ("FROM CompUser
o") and the expected SQL in the criteria tests (which names the table
CompUser) both stay as they are.

Reproduced on mariadb:11.8 by running the jpql expressions and functions
packages first and the criteria package second, on a freshly created
database: the jpql phase is 123 tests and green either way, and the criteria
phase goes from 540 tests with 133 failures and 183 errors, 513 of them
reporting "Row size too large", to 544 tests with none. The new test states the
invariant directly and fails without the change.

The MariaDB behaviour is user facing on its own, independent of the test
suite: an application configured with
SynchronizeMappings=buildSchema(SchemaAction='drop,add'), or a deployment
script running mappingtool -action refresh, rotates the same columns on every
restart and eventually fails the same way on a table whose visible columns fit
comfortably inside the limit, with nothing in information_schema to hint at
it. It is therefore written up as a third item under "Known issues with
MariaDB" in supported_databases.xml, naming the remedies - rebuild the table
with ALTER TABLE ... FORCE or OPTIMIZE TABLE, or do not use a drop/add schema
action against a long-lived MariaDB schema - and cross referencing the runtime
forward mapping section. The file is still well formed: xmllint --noout
openjpa-project/src/doc/manual/supported_databases.xml.
… fails

junit.framework.TestCase.runBare() calls setUp() outside its try/finally:

  setUp();
  try { runTest(); } finally { tearDown(); }

so a failure in setUp() skips tearDown() altogether. CriteriaTest.setUp()
opens an EntityManager, and a subclass such as TestMultiselect then begins a
transaction in its own setUp() to clear the tables, which makes the store
manager retain a JDBC connection for the rest of that transaction. When that
step failed, the EntityManager and its connection were never released, and
because the criteria base classes keep the EntityManagerFactory in a static
field shared by every test class of their hierarchy the leaked connections
accumulated in one pool.

The test profiles size that pool at MaxTotal=10 with MaxWait=10s, so about a
dozen such failures emptied it and every later test in the same JVM failed
with

  PersistenceException: Cannot get a connection, pool error Timeout waiting
  for idle object, maxWaitDuration=PT9.999990575S

rather than with its own problem - which is how one broken schema
synchronisation turned into 174 failures and 185 errors in the scheduled
Databases run, with TestMultiselect spending 170 seconds purely on pool
timeouts.

AbstractCriteriaTestCase.runBare() now runs setUp() inside the guarded block
and calls tearDown() exactly once either way, which is what JUnit 4 does for
@Before/@after. For that to help, every criteria base class has to release
what setUp() opened, so the release sequence is hoisted into
AbstractCriteriaTestCase.releaseResources(): close the EntityManager, clear
the audited SQL, close the EntityManagerFactory, drop the references, each
step in its own finally so that a failure releasing one resource cannot leave
the next one open. CriteriaTest, EmbeddableDomainTestCase and
JoinDomainTestCase all delegate to it; previously only one of the three was
hardened, and EmbeddableDomainTestCase did not even reach
AbstractCriteriaTestCase.tearDown().

TestEmbeddableCriteria deliberately keeps its factory open for the whole
class, because rebuilding it would re-run buildSchema(SchemaAction='drop,add')
over the embeddable domain once per test method, and it did so by counting
test methods in setUp() and closing the factory in tearDown() only on the
last one. With tearDown() now running after a failed setUp() as well, that
counter would stop advancing at the first failure and the factory would never
be closed again, so the methods are counted in tearDown() instead, the count
is taken from the class rather than hard coded, and the EntityManager is
released on every method regardless.

This is deliberately a fix to the test infrastructure and not a larger pool
or a longer wait, either of which would only postpone the symptom.

TestSetUpFailureReleasesConnection drives three probes. Fourteen of them -
more than the pool holds - fail in setUp() after taking a connection in a
transaction that writes, and, unlike a real criteria test, release only their
own EntityManager so that one factory and one pool survive all fourteen; the
test affirms after each probe that its EntityManager was closed and at the
end that the pool still hands out a connection. A second probe closes
cleanly but throws out of EntityManager.close(), and affirms that the factory
was closed all the same. A third repeats the first failure in the
EmbeddableDomainTestCase hierarchy. The probes live in SetUpFailureProbes
rather than nested in the Test* class, or surefire reports their deliberate
failures as failures of the build whenever the criteria package is selected
by path.

Without the runBare() change, the first probe and the EmbeddableDomainTestCase
probe fail immediately:

  junit.framework.AssertionFailedError: probe 0 left the EntityManager its
  setUp() opened behind, so the connection it retained never went back to
  the pool

and with that assertion removed as well, so that the probes keep leaking, the
loop reproduces the CI symptom directly - a later probe no longer gets a
connection at all:

  org.apache.openjpa.persistence.PersistenceException: Cannot get a
  connection, pool error Timeout waiting for idle object,
  maxWaitDuration=PT9.999993S

Verified: the whole criteria package, 544 tests, no failures, on Derby and on
H2 (-Dtest-h2).
@rzo1 rzo1 self-assigned this Oct 13, 2026
@rzo1
rzo1 requested review from cristof and solomax October 13, 2026 12:56
* relies on holds for each of them.
*/
protected final void releaseResources() throws Exception {
try {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this "cascade" can be breaked into several private methods to improve readability?

try {
tearDown();
} catch (Throwable tearingDown) {
if (exception == null) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need addSuppressed here?

@solomax

solomax commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

it seems weekly build waits for this one to pass :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants