On Wed, Feb 19, 2020 at 4:26 PM GitBox <g...@apache.org> wrote: > mads1980 commented on issue #246: OpenSSLEngine improvements to guard > against multiple shutdown() calls triggered by construction exception and > finalize() later > URL: https://github.com/apache/tomcat/pull/246#issuecomment-588286325 > > > > Well, personally I don't approve until there's an explanation. > > > > * I still don't see what the atomic boolean brings > > OpenSSLContext has a very similar protection (this is were I got the > idea for this solution), but it uses AtomicInteger instead of AtomicBoolean > (not sure why, since AtomicBoolean is more semantically "readable", and > both implementations internally use a volatile int for storage, so memory > usage is the same). > > > all relevant code is already synchronized and the check on the > boolean flag should be identical. > > The fact that shutdown() is synchronized does not prevent it from > executing concurrently with a failed constructor. There is no > synchronization within the constructor, while shutdown() is being > concurrently execute as a result from finalize() being invoked by the JVM > GC concurrently with the failed constructor. > > > I suppose the actual "fix" is the check if (networkBIO != 0). > > Actually the HotStop error log shows the stack trace on freeSSL() but I > guess that it does not hurt protecting both networkBIO and ssl from null > pointer frees. > > > * "This can happen if there are uncaught exceptions within the > OpenSSLEngine constructor, as finalization can execute concurrently with > object construction": ok, so what is the uncaught exception ? > > This is not being captured by the HotSpot error logs, since a SIGSEGV > crash occurs before any useful logging. However, it must be some kind of > exception arising from the JNI calls within the OpenSSLEngine constructor. >
GitHub has decided I cannot comment, so posting it here. I still cannot reproduce this with the GC config (which is missing -XX:+UnlockExperimentalVMOptions). As with https://bz.apache.org/bugzilla/show_bug.cgi?id=63802 I will not add weird fixes for issues that don't exist. So I don't understand what difference the AtomicBoolean makes, what "matters" to me here is the != 0 check in shutdown. The AtomicInteger in OpenSSLContext is not to be used for comparison and comes almost straight from some Netty code (it was an AtomicIntegerFieldUpdater which got changed for some reason). I don't quite see why it's needed either over a simple boolean since it's all synced, but there it's really not used at all so it stays. Rémy > > ---------------------------------------------------------------- > 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. > > For queries about this service, please contact Infrastructure at: > us...@infra.apache.org > > > With regards, > Apache Git Services > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > >
