rzo1 commented on PR #2846:
URL: https://github.com/apache/tomee/pull/2846#issuecomment-5318835738
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 — including one correction where the refutation pass ran the real classes
and disproved the reviewer's stated failure mode, which turned out to be the
most interesting result here. Take it as input, not as a verdict — I have not
run the build.
**Overall: the core idea is sound and the risky parts hold up.** Moving the
marking from the end of `createApplication` into the end of `startEjbs` is
genuinely necessary — all the `comp/EJBContext`, `comp/WebServiceContext` and
`comp/TimerService` bindings happen in `Container.deploy()`, which runs inside
`startEjbs`. Nothing between `startEjbs` and the end of `createApplication`
writes into a bean ENC or the app JNDI context (`bindGlobals`,
`postConstructResources`, the app-client loop, and the
`AssemblerAfterApplicationCreated` observers in
WsService/RESTService/Hessian/BatchEE all target the container-system global
context or only read). Each component context is a fresh `IvmContext` root, so
`setReadOnly` cannot leak into a shared tree; the `ContextHandler` wrapper
delegates writes to the underlying `IvmContext` so enforcement still applies;
undeploy only unbinds from the global context; and the flag defaults off. The
`AtomicInteger` races are benign.
What does not hold up is the mechanism for deciding when the *shared
application* context can be closed.
### 1. The `pendingLateModules` counter never drains when a deployment pass
has no EJBs (minor)
`setAppNamingContextReadOnly()` locates the AppContext via the new
`appContextOf(allDeployments)` helper, which returns `null` for an empty list —
and then returns `false` *before* calling `appContext.lastModuleDeployed()`. So
the counter is only decremented by passes that happen to contain at least one
`BeanContext`, while `setPendingLateModules(appInfo.webAppAlone ? 0 :
appInfo.webApps.size())` assumes exactly `1 + webApps.size()` passes.
Both ends break on ordinary packagings:
- The `createApplication` pass is empty whenever the EAR has no EJB module
in `lib` — `isSkip` (`Assembler.java:1642`) skips every `ejbJar.webapp == true`
module when `webappId == null`. So an EAR whose beans live only in its WARs
produces N passes for a count of N.
- A late pass is empty whenever an EAR web module has no EJBs (a plain
JSF/servlet WAR). `TomcatWebAppBuilder.java:1468` still calls
`assembler.startEjbs(true, beanContexts)` unconditionally, but with an empty
list — and a WAR with no EJBs and no `beans.xml` gets no synthetic `.Comp` bean
either, since `AnnotationDeployer` only adds `CompManagedBean` when `beans !=
null`.
Counting passes rather than tracking which modules are outstanding is the
root cause. The AppContext is available to `startEjbs` through the caller in
every pass, so it does not need to be recovered from the bean list.
**Important correction to the impact**, because the first pass got this
wrong and the refutation pass disproved it by running the real classes:
`java:app` is *not* left writable to applications. `EnterpriseBeanBuilder:153`
binds the shared `java:app` subcontext into every bean ENC, and
`NameNode.setReadOnly` recurses into `Federation` members
(`NameNode.java:300-311`), so marking any single bean ENC already flags the
shared `java:app` subtree read-only. An EJB doing `new
InitialContext().lookup("java:app").createSubcontext("x")` gets
`OperationNotSupportedException` even when the counter never drained. What
actually leaks is only the root `AppContext.getAppJndiContext()` handle,
reachable via `javaURLContextFactory` when there is no `ThreadContext` and the
TCCL is the app classloader — a narrow, fail-open gap in an opt-in feature.
Hence minor, not major.
Note the added unit test cannot catch this:
`testAppContextWaitsForEveryLateModule` drives `lastModuleDeployed()` directly
and always passes a non-empty bean list.
### 2. That transitive marking means the counter machinery is largely
redundant (minor — design)
Following from the verification above: since the first `startEjbs` pass
already makes `java:app` read-only for every application-visible path, the
`AtomicInteger` pass counter, the new `AppContext.lastModuleDeployed()` API and
the tests built around them buy nothing except deferring the flag on the root
context object. The documented invariant "the application context still accepts
the container's own bindings" happens to hold only because
`JndiBuilder.bindJava` binds through the root instance, which never checks the
node-level flag.
Verified directly: after `comp.setReadOnly(true)` on a comp context that has
`"app"` bound, a write reached as `comp → app` throws
`OperationNotSupportedException`, while binds issued on the shared app root
`IvmContext` still succeed.
So rather than fixing the counter per finding 1, it may be worth deleting it
and marking the app context on the first pass (or at least correcting the
javadoc to describe what is actually enforcing the invariant). That would
remove both this and finding 3.
### 3. The embedded path skips `startEjbs` when CDI is off (nit)
`LightweightWebAppBuilder.deployWebApps()` calls `assembler.initEjbs(...)`
for every web module when `!webAppAlone` (line ~146) but only calls
`assembler.startEjbs(true, beanContexts)` inside `if (!appInfo.webAppAlone &&
hasCdi(appInfo))` (line ~180). Since the patch hangs the "last module in"
decision exclusively off `startEjbs`, an EAR deployed embedded with CDI
disabled never reaches the final pass. Same root cause as finding 1 on a
different call site, and it makes the feature behave differently between the
embedded and Tomcat runtimes. Nit rather than minor because in that
configuration those bean contexts are never deployed into their containers at
all (a pre-existing embedded quirk), and the federated `java:app` view is still
read-only.
### 4. Two vacuous test assertions and a duplicated test (nit)
`JavaCompReadOnlyTest.assertWriteRefused` only fails when an exception
*other than* `OperationNotSupportedException` escapes, so a write that silently
succeeds is accepted and the invariant is delegated to the follow-up
`assertNotBound`/`lookup` checks. That is fine for
bind/rebind/createSubcontext, but `IvmContext.rename` (`IvmContext.java:378`)
and `IvmContext.destroySubcontext` (line 412) throw
`OperationNotSupportedException` *unconditionally*, with no `readOnly` check —
so those two lines would pass with the feature completely removed. Either drop
them or assert them against a writable context too.
Separately, `testReadOnlyAppNamingContext` and
`testAppNamingContextReadOnlyWhenEnabled` in `AppNamingReadOnlyTest` are
byte-for-byte identical.
--
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]
