ChristopherSchultz commented on pull request #456:
URL: https://github.com/apache/tomcat/pull/456#issuecomment-955018111
Your test-case looks even more complicated than necessary: just initialize
two of them then deinitialize them. No shutdown hook necessary, right?
Okay, so it's not crashing in the `AprLifecycleListener`'s shutdown, which
is what it sounded like you were reporting.
Certainly, anything can crash at any time after the APR global pools have
been shut-down. We could put guards around those things. Something like this at
the top of each of the calls which require APR:
```
#DEFINE CHECK_APR_INITIALIZED(ENV) { \
if(!tcn_global_pool) { \
tcn_ThrowAPRException((ENV), APR_EINIT); \
} \
} \
TCN_IMPLEMENT_CALL(jstring, Address, getnameinfo)(TCN_STDARGS,
jlong sa, jint flags)
{
apr_sockaddr_t *s = J2P(sa, apr_sockaddr_t *);
char *hostname;
UNREFERENCED(o);
CHECK_APR_INITIALIZED(e); /* <- This macro invocation is new */
if (apr_getnameinfo(&hostname, s, (apr_int32_t)flags) == APR_SUCCESS)
return AJP_TO_JSTRING(hostname);
else
return NULL;
}
```
It would be much cleaner to implement this at the Java level, but as your
test-case demonstrates, it's always possible for APR to disappear during the
execution of one of the native methods.
I'm kind of curious as to exactly where in `Socket.accept()` this particular
failure occurred. We use APR pools for things like string values, even to throw
exceptions. So it's hard to avoid using APR anywhere. One could argue that APR
pools aren't necessary for throwing exceptions, but re-writing tcnative at this
point to remove APR-type things is unlikely. Perhaps incrementally. But Rémy's
recent work with Project Panama looks like we might be able to dump tcnative in
the reasonably-near future.
Anyway, back to fixing this kind of thing:
1. I think it's worth mentioning the dangers of multiple
AprLifecycleListeners in the Tomcat documentation. I don't think the PR as
submitted goes far enough. I think it's even misleading at first (e.g. only use
`AprLifecycleListener` on a `Server` element) because there are many ways to
use the `AprLifecycleListener` in ways that can cause the JVM to crash. I think
this documentation should be changed in the Javadoc but also in the
documentation for the listener in general, here
http://tomcat.apache.org/tomcat-10.0-doc/apr.html#APR_Lifecycle_Listener_Configuration
and here
http://tomcat.apache.org/tomcat-10.0-doc/config/listeners.html#APR_Lifecycle_Listener_-_org.apache.catalina.core.AprLifecycleListener.
(Note that the second of these two already states that the listener should
only be used on `<Server>` components.)
1. I think it's also reasonable to try to protect the JVM against this kind
of failure. Anyone starting two Tomcats in a single JVM is going to have
exactly the same problem. Yes, it's possible to have the "container" (e.g.
Spring in this case) manage the whole, um, _lifecycle_ of the
`AprLifecycleListener` but it's much more natural to use it "inside" Tomcat and
allow Tomcat to manage that process.
I like the idea of reference-counting, especially because the number of
times the `AprLifecycleListener` is initialized and de-initialized in a given
JVM should be relatively low. It's a small amount of code to manage, provides a
great amount of protection (JVM crashes should really never be tolerated), and
the performance impact is irrelevant.
Would you care to prepare a reference-counting patch for
`AprLifecycleListener` either as a part of this PR, or as a separate one?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
