jungm commented on PR #2850: URL: https://github.com/apache/tomee/pull/2850#issuecomment-5108932683
Thanks for the careful review — both points confirmed, and both are addressed in e4e60f8. **1. ERROR-level NPE noise on the pre-`startEjbs` rollback — fixed.** Reproduced exactly as you described. The root cause is an asymmetry: `EjbJarBuilder:83` assigns the container at *build* time, while `containerData` is only set inside `Container.deploy()`. So after a CDI failure the rollback holds a non-null container for a bean the container never saw, and `SingletonInstanceManager.freeInstance:216` (`data.singleton`) plus `StatefulContainer.undeploy:292` (`data.jmxNames`) both dereference a null `Data`. Notably `SingletonInstanceManager.undeploy:341`, `StatelessInstanceManager.undeploy` and `ManagedContainer.undeploy` already have precisely this guard — these two were simply the ones that lacked it, so the fix follows the existing idiom rather than inventing one. I swept every `Container.undeploy` implementation; no others are missing it. A third variant showed up once those were guarded: `Assembler`'s stop/undeploy loops NPE'd on `getContainer()` itself for beans whose container had already been cleared, so those are null-checked too. The rollback is now completely silent — 0 NPEs. **2. Webapp smoke test — done, and it is not a regression.** Deployed a war with an unsatisfied `@Inject` (plus a stateful and a singleton bean) into a real TomEE Plus, then ran the same war against a pristine pre-fix `openejb-core` for a controlled A/B on freshly unpacked servers: | | pristine | with fix | |---|---|---| | deploy attempts | 1 | 1 | | `NullPointerException` | 0 | 0 | | `Error destroying child` | 1 | 1 | | `invalid Lifecycle transition` | 1 | 1 | | SEVERE total | 4 | 4 | The normalized SEVERE lines are identical between the two; only their ordering shifts, since the rollback now runs slightly earlier relative to `TomcatWebAppBuilder`'s own message. The `removeChild`/`before_destroy` interaction you spotted is real, but it is **pre-existing upstream behaviour** on the webapp path — `TomcatWebAppBuilder`'s catch at :1341 already undeploys before the caller does, and the second undeploy is idempotent as you suspected. The server stays healthy afterwards (`/` returns 200, the failed app is correctly absent). Worth noting I first measured 2× these counts and had to discard that run — a stale exploded dir in `work`/`temp` had caused the war to deploy twice, so the control was invalid rather than the fix. **3. Nit — fixed.** The test now catches `DeploymentException` rather than `Exception`, so it still guards the branch it was written for. Added `undeployingABeanThatWasNeverDeployedIsQuiet`, which fails without the container guards with `NullPointerException: Cannot read field "jmxNames" because "data" is null` and passes with them. One thing to flag: `StatefulDecoratorInjectionTest`, `StatefulConversationScopedTOMEE1138Test` and `StatefulDependentInjectionTest` fail in `openejb-core`, but they fail identically on pristine `a3e8806a9d` with no changes applied, so they are unrelated to this PR. _🤖 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]
