jungm commented on PR #2846: URL: https://github.com/apache/tomee/pull/2846#issuecomment-5109222209
Thanks — this was a genuinely useful review, and you were right on every point. Pushed in 5cf5d33. **Timing.** Moved as you suggested, but the end of `initEjbs` turned out to still be too early: the containers bind `comp/EJBContext`, `comp/WebServiceContext` and `comp/TimerService` into each bean ENC from `SingletonInstanceManager.deploy()`/`StatelessInstanceManager.deploy()`, which run in `startEjbs`. Marking at the end of `initEjbs` made `JavaCompReadOnlyTest` fail to deploy at all with `Failed to bind EJBContext/WebServiceContext/TimerService` — so it would have broken every singleton and stateless deployment. The marking now happens at the end of `startEjbs`, with the intent recorded on the `AppContext` at configuration time, so the late modules from `TomcatWebAppBuilder` are covered and all container-internal binds have run first. **The shared app context.** Since `initEjbs`/`startEjbs` run once per web module for an EAR, closing `appContext.getAppJndiContext()` on the first pass would have broken the later ones in exactly the way you described. `AppContext` now carries a count of the late modules still to come (`appInfo.webAppAlone ? 0 : appInfo.webApps.size()`) and only closes the app context on the final pass. `AppNamingReadOnlyTest.testAppContextStaysWritableUntilTheLastModule` and `testAppContextWaitsForEveryLateModule` pin that. **Dead `WebContext` loop.** Confirmed and removed — `setJndiEnc` gets `new InitialContext()` from `TomcatWebAppBuilder` or a `WebInitialContext` proxy from `LightweightWebAppBuilder`, never an `IvmContext`/`ContextHandler`. You're also right that the PR body's claim about it fixing the web vehicles was unsupported; the web-tier TCK writes pass because the EJB contexts are marked. I've corrected the description. The remaining web-tier read-side gap is filed as TOMEE-4658. **Opt-out.** Now reads `appInfo.properties` first with the system property as fallback, matching `OPENEJB_TIMERS_ON`, and parsed with `Boolean.parseBoolean` so `=FALSE` is honoured. **`EmbeddedTomEEContainerTest`.** Inverted to `testEjbCannotCreateSubContextByDefault`, accepting either refusal mode. **Test nits.** `assertWriteRefused` no longer requires `OperationNotSupportedException` (it tolerates both outcomes and the not-observable assertion carries the weight), `deploy()` moved inside the try/finally, tautological `rename`/`destroySubcontext` assertions replaced, and the `if(`/`for(` spacing fixed in the new code. Full `openejb-core` suite: 4096 tests, 6 failures, all pre-existing security ones that reproduce on a clean `main`. One run also failed `ConnectionFactoryTxTest` with a null injected `ConnectionFactory`, which looked like a plausible consequence of a read-only ENC — but `InjectionProcessor` performs no ENC writes, it passed 5 isolated runs and a repeat full run, so it is a pre-existing flake against the shared broker rather than fallout from this change. Agreed on the Arquillian point; the inverted test above is the real-container coverage, and I'm happy to add more if you'd like a specific EAR-with-WAR scenario. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_ -- 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]
