rzo1 commented on PR #2847:
URL: https://github.com/apache/tomee/pull/2847#issuecomment-5318835173
Currently short on time, so AI review only: I ran an adversarial review pass
over this PR, followed by a second pass whose job was to *refute* the first
one's findings against the actual code. Everything below survived that second
pass (two findings did not and are listed at the end as explicitly dismissed).
Take it as input, not as a verdict — I have not run the build.
**Overall: small and mostly right.** `Assembler.destroyApplication` (~line
2552) is the only production caller of `ReloadableEntityManagerFactory.close()`
in the tree, and the previous behaviour meant an application-initiated close
made undeploy throw — which also skipped
`persistenceClassLoaderHandler.destroy(...)` and `remf.unregister()`, leaking
the transformer registration and the MBean. Guarding fixes that, and the
delegate `isOpen()` semantics hold for OpenJPA
(`!DelegatingBrokerFactory.isClosed()`), EclipseLink and Hibernate, so no
distribution-specific breakage. There is no second half in tomee-catalina:
`TomcatWebAppBuilder` only calls `overrideClassLoader`/`createDelegate`, never
`close`. The new test is non-vacuous — it fails against unpatched code.
My reservation is placement, not intent.
### 1. The guard is on the public EMF contract rather than the one call site
(minor)
`ReloadableEntityManagerFactory` is not an internal-only helper: it is the
object bound at `openejb/PersistenceUnit/<id>` (`Assembler:931`), and
`JndiEncBuilder:396-406` binds a plain `IntraVmJndiReference` to that same name
for every `persistenceUnitRef` with no wrapper, while
`TomcatJndiBuilder.mergeRef(PersistenceUnitReferenceInfo)` (~:607) does
`setResource(resource, factory)` with the raw REMF. So an application injecting
`@PersistenceUnit EntityManagerFactory` really does hold this instance.
(Contrast `persistenceContextRef`s, which are wrapped in `JtaEntityManager` —
only the EMF ref exposes the REMF.)
`jakarta.persistence.EntityManagerFactory#close()` is specified to throw
`IllegalStateException` when already closed, and every provider TomEE ships
honours that. After this patch, that exception is gone for application callers
too, not just for `Assembler.destroyApplication`.
Since the problem exists at exactly one production call site, the
containable fix is there — `if (remf.isOpen()) { remf.close(); }`, or a
try/catch around that single call — which restores the undeploy path without
changing the contract of a public `EntityManagerFactory` implementation.
Two things the refutation pass corrected in the original write-up, so nobody
over-weights this: closing a container-managed EMF from application code is
itself illegal per the EE/JPA contract, so the swallowed exception only occurs
on already-illegal usage. And the TCK argument in the PR body cannot be
evaluated from this repo — there is no JPA TCK harness under `tck/` (bval, cdi,
concurrency, data, jsonb, jsonp, security, jax-rs, microprofile) and no
`persistence-javatest.txt` in the tree — so "the exclusion can be cleared once
this merges" is unverifiable here. If that exclusion lives elsewhere, it would
be good to confirm the vehicle does not assert the `IllegalStateException` this
patch now swallows.
### 2. The patch removes only one of several ways undeploy skips
`destroy()`/`unregister()` (minor)
`Assembler.java:2547-2557` wraps `remf.close()`,
`persistenceClassLoaderHandler.destroy(unitInfo.id)` and `remf.unregister()` in
a *single* `try/catch (Throwable)`. Any failure inside a provider's `close()` —
not just the already-closed `IllegalStateException` this PR guards — still
jumps to the catch and leaks the classloader transformer registration and the
JMX MBean for that persistence unit.
Giving `remf.close()` its own try/catch, or reordering so
`destroy()`/`unregister()` always run, is what actually makes undeploy robust —
and it is the same one-call-site edit that avoids touching the public `close()`
contract in finding 1. Worth doing both in one go.
### 3. `close()` leaves the closed delegate in place (nit, pre-existing)
`ReloadableEntityManagerFactory.close()` never nulls `delegate` nor marks
the factory closed, so `delegate()` still returns the non-null closed instance
afterwards and `createEntityManager()`/`getProperties()`/`getCache()` hand
callers a closed provider EMF that fails deep inside the provider rather than
at the boundary. Pre-existing rather than introduced here, but it is the same
object-lifecycle gap the PR is papering over — clearing the field in `close()`
would make the new guard unnecessary.
### Checked and dismissed
- **TOCTOU on the volatile `delegate` field** (three reads in `if (delegate
!= null && delegate.isOpen()) { delegate.close(); }`). Pre-patch `close()`
already did two independent reads, so the "read non-null → `createDelegate()`
swaps → close the new delegate" interleaving was already possible; the extra
`isOpen()` read changes nothing about which instance can be closed. The named
concurrent caller does not fit either: `TomcatWebAppBuilder:1423-1435` only
calls `createDelegate()` when
`unitInfo.webappName.equals(webAppInfo.moduleId)`, i.e. for the one webapp that
owns that PU during its own deployment. Capturing the field in a local is a
fine tidy-up, but it is not a defect this PR creates.
- **The new test's quality.** It is non-vacuous (2 close calls vs 1 against
unpatched code) and well-formed — `EntityManagerFactoryCallable`'s 5-arg
constructor, `PersistenceUnitInfoImpl.setLazilyInitialized` and
`Reflections.set` all match, and `lazilyInitialized` keeps the constructor from
building a real EMF, so it exercises precisely the changed branch. Demanding an
`Assembler.destroyApplication` integration test for a two-token guard would be
disproportionate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
