rzo1 commented on PR #2850:
URL: https://github.com/apache/tomee/pull/2850#issuecomment-5088904524
Correct diagnosis and the minimal fix. Confirmed the chain: the unwrapped
`DeploymentException` really does come from
`ThreadSingletonServiceImpl:259`, `initEjbs`
really does register ids before `new CdiBuilder().build(...)`, and the
message key
`createApplication.undeployFailed` already exists at
`Messages.properties:76`. Ran the
test both ways — unpatched it fails with
`the failed deployment leaked its deployment id expected null, but
was:<BeanContext(id=TheSharedDeploymentId)>`.
One thing I'd like fixed before merge, which my local run surfaced:
- Rolling back a failure that happens *before* `startEjbs` now drives
`Container.stop/undeploy` over `BeanContext`s that were never deployed
into their
container, which emits ERROR-level NPEs (e.g.
`SingletonInstanceManager:216`) for every
singleton/stateful bean. Since a CDI bootstrap failure is now the most
common path
through this branch, that means the common failure mode gets a wall of
ERROR-level
noise stacked on top of the real cause. Guarding `destroyApplication` on
whether
`startEjbs` ran, or making the container stop tolerant of an undeployed
BeanContext,
would fix it.
And one I couldn't prove but would want smoke-tested:
- For webapps, `destroyApplication` calls
`webAppBuilder.undeployWebApps(appInfo)`, which
ends in `host.removeChild(standardContext)`
(`TomcatWebAppBuilder:1663-1690`), plus
`ClassLoaderUtil.destroyClassLoader`. `createApplication` for a webapp
runs on the
Tomcat context-start thread, and `TomcatWebAppBuilder`'s own catch at
:1341 then calls
`undeploy(...)` again. So this newly routes the most common webapp
deployment failure
into an in-flight Catalina child removal plus classloader destruction,
followed by a
second undeploy from the caller.
The pre-existing `catch (Throwable)` branch already does exactly this for
other
failures and the second undeploy looks idempotent (`findChild` returns
null), so I
suspect it's fine — but it's the one behavioural change here that reaches
outside
openejb-core. Could you deploy a war with an unsatisfied `@Inject` into a
real TomEE
and confirm the log is clean? The openejb-core unit test can't see this.
Nit: `catch (final Exception expected)` in the test is broad enough that it
would still
pass if the deployment started failing for an unrelated reason. Asserting on
`DeploymentException` would keep it guarding the branch you actually fixed.
--
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]
