On 24/01/2019 15:22, Rainer Jung wrote: > Checked in as 1852036. My tests looked good (no crashes), but it would > be useful to rerun the original memleak checks against the patched code.
Will do. Mark > > Regards, > > Rainer > > Am 24.01.2019 um 15:54 schrieb Rainer Jung: >> Am 24.01.2019 um 15:51 schrieb Rainer Jung: >>> Am 24.01.2019 um 14:16 schrieb Mark Thomas: >>>> On 24/01/2019 13:01, Rainer Jung wrote: >>>>> Am 24.01.2019 um 13:37 schrieb Mark Thomas: >>>> >>>> <snip/> >>>> >>>>>> It can happen in normal usage. I saw it once. It happens on Connector >>>>>> stop so it could be worse. My memory leak fix is definitely the root >>>>>> cause. Any thoughts on a possible fix? >>>>> >>>>> Currently experimenting with a cleanup handler registered for >>>>> con->pool. >>>>> That can get an arbitrary argument and is called when the cleanup >>>>> happens, especially useful for the problematic cleanup as a child >>>>> pool. >>>>> The handler could overwrite con->pool with NULL. But that would only >>>>> make sense, if we are sure, that we are notgoing to use stuff after >>>>> OpenSSLContext.destroy(). >>>> >>>> That handler sounds like the way to go. Remove the reference to the >>>> pool >>>> when it is cleaned up. >>>> >>>> I'm reasonable sure that there isn't any usage of the pool after that >>>> point else we'd be seeing crashes all over the place. >>> >>> I needed a few attempts due to the following complexities: >>> >>> - using con as data in the cleanup handler and resetting con->pool >>> didn't work, because con is allocated from that pool and when we >>> later check con->pool the contents of con are already undefined. >>> Various errors happen. >>> >>> - so I decided to use ssl as the data. But ssl is of type SSL which >>> can only be extended via app data. So I needed to introduce a new >>> index 4 tacking destroyCount similar to handshakeCount in index 3. >>> >>> - I also moved the SSL_free(ssl_) in freeSSL() behind the pool >>> destruction to ba safe, in case that SSL_free would somehow influence >>> the app data stored in the ssl. con is part of that app data and so >>> con->pool. >>> >>> We could combine eg. handshakeCount and destroyCount in one struct >>> and get ack to only 3 app data indexes, but since their use is very >>> different, I prefer the 4th index. >>> >> >> So I ended up with the following patch: >> >> https://home.apache.org/~rjung/patches/tcnative-con-pool-lifecycle.patch >> >> Regards, >> >> Rainer > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
