michael-o commented on pull request #456:
URL: https://github.com/apache/tomcat/pull/456#issuecomment-955175606
> Your test-case looks even more complicated than necessary: just initialize
two of them then deinitialize them. No shutdown hook necessary, right?
This maybe true, but it replicates the behavior in Spring Boot. This is what
I wanted to show you.
> Okay, so it's not crashing in the `AprLifecycleListener`'s shutdown, which
is what it sounded like you were reporting.
I don't think I have actually written that. It is the APR protocol/endpoint.
> 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;
> }
> ```
This sounds like a decent idea. It makes post mortem analysis much easier.
It is work, but just manual one.
> 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.
Agreed.
> 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.
Unlikely since Tomat 9 and 10 will live very long and the minimum Java
version is 8. Expect tomcat-native as of now to live for at least five more
years. A future version with Panama will likely to have OpenSSL only, for now.
> 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.)
This is the first problem, the documentation you mention is intended for
integrators and admins. Embedders read Javadoc only, as you have seen with
Spring Boot. Wen should add at least minimal information to all of these
listeners' Javadoc and maybe a link to the extended documentation. (our fault)
What would be your proposal for the documentation.
> 2. 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.
Yes, natural as long as you adibe and know the contract and requirements
which partially did not happen in Spring Boot. I am inclined to add a WARNING
log to all of these listeners when the `Lifecycle` is not of instance `Server`.
> 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?
I would like to take care of the documentation shortage first and then
discuss in a separate PR/thread reference counting approach.
--
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: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
