rzo1 commented on PR #2848:
URL: https://github.com/apache/tomee/pull/2848#issuecomment-5088904912
The direction is right and the codebase already agrees with you —
`FinderFactory.createFinder` (:235-243) swallows CNFE for these same servlet
classes, the
taglib listener loop two blocks below only logs, and the EJB path at :2447
does `continue`.
The three fatal throws were the odd ones out. The `{1}{2}{3}` -> `{0}{1}{2}`
MessageFormat
fix is a real bug fix on its own. And the test genuinely regresses:
reverting only
`AnnotationDeployer.java` gives `OpenEJBException: Unable to load servlet
class: ...TestServlet1`.
But the reported bug is not actually fixed on half the distributions:
- `ConfigurationFactory:146` defines `WSDL4J_AVAILABLE`, and :260-262 adds
`new WsDeployer()`
to the deployer chain whenever `openejb.webservices.enabled` (default
true) &&
`WSDL4J_AVAILABLE`. `WsDeployer.processPorts(WebModule)` iterates every
servlet of the
WebApp — only skipping `className == null` for JSPs — and does
`webModule.getClassLoader().loadClass(className)` at ~:152, inside a try
whose catch at
:233-235 is `catch (final Exception e) { throw new
OpenEJBException("Unable to load servlet class: " + className, e); }`.
The `loadClass` precedes the `JaxWsUtils.isWebService(clazz)` test, so it
is not limited
to WS endpoints. `libre-wsdl4j` is optional in openejb-core's pom
(:612-616) but is present
on Plus, Plume and openejb-standalone. So on those distributions the war
that motivated
this ticket still fails to deploy, with an identically worded error from a
different class.
Note when you fix it that `NoClassDefFoundError` is an `Error`, so `catch
(Exception)`
there doesn't catch it at all — it propagates raw today.
Consequently the test can't do the job it's meant to do:
- It drives `AnnotationDeployer` directly, so it would have stayed green
through exactly the
gap above. Something at `ConfigurationFactory.configureApplication` level,
or an Arquillian
test with a real war, is what would actually pin TOMEE-4642.
Other things:
- The caught `Throwable` is discarded entirely — `logger.warning("Unable to
load servlet class: " + servletClass)`
drops both the cause and the module identity. Whoever hits this because
they genuinely
mis-packaged a war now gets one line and no stack trace, on a path that
used to be fatal
and loud. Please pass `e` and include `webModule.getJarLocation()`.
- The Servlet 6.1 §2.3.1 rationale in the comment covers servlets, whose
loading may be
deferred. It does not cover filters or listeners — the container must
instantiate those at
context start, so a missing class there is unambiguously broken, just
broken later. Worth
either reflecting that in the comments or keeping a stronger log level for
those two.
- Relatedly, `LightweightWebAppBuilder:183-186` now regresses to a raw
`ClassNotFoundException`
mid-`createApplication` instead of the clean `OpenEJBException` it used to
get.
- Fail-open side effect worth acknowledging: an unloadable servlet class now
silently skips
`@Resource`/`@EJB` annotation processing for that servlet, so if the class
*is* resolvable
later by a different loader it comes up without its injections rather than
not at all.
- The MessageFormat index fix is applied to 4 of 12 identical occurrences in
this file —
:2019, :2044, :2061, :2192, :2349, :2379, :2929 and :5696 have the same
off-by-one. Either
do them all or split that into its own trivial commit.
- Twenty lines below your last hunk, the webservices handler-chain loop
still does
`throw new OpenEJBException("Unable to load webservice handler class: " +
handlerClass, e)`
from the identical catch shape. Keeping a declared handler fatal is
defensible, but the
method now has three tolerant sites, two `logger.error` sites and one
fatal site with
nothing saying why. One comment would do.
--
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]
