Ensure we only add OpenSslEngine to the OpenSslEngineMap when handsha… #4653
Conversation
…ke is started Motivation: We need to ensure we only add the OpenSslEngine to the OpenSslEngineMap when the handshake is started as otherwise we may produce a memory leak when the OpenSslEngine is created but not actually used. This can for example happen if we encounter a connection refused from the remote peer. In this case we will never remove the OpenSslEngine from the OpenSslEngineMap and so it will never be collected (as we hold a reference). This has as affect that the finalizer will never be run as well. Modifications: - Lazy add the OpenSslEngine to the OpenSslEngineMap to elimate possible leak. - Call OpenSslEngine.shutdown() when SslHandler is removed from the ChannelPipeline to free memory asap in all cases. Result: No more memory leak with OpenSslEngine if connection is refused.
|
@nmittler @Scottmitch @trustin PTAL... This is a serious bug when you use OpenSslEngine on the client-side and result in a memory leak. |
| @@ -429,6 +429,10 @@ public void handlerRemoved0(ChannelHandlerContext ctx) throws Exception { | |||
| // Check if queue is not empty first because create a new ChannelException is expensive | |||
| pendingUnencryptedWrites.removeAndFailAll(new ChannelException("Pending write on removal of SslHandler")); | |||
| } | |||
| if (engine instanceof OpenSslEngine) { | |||
There was a problem hiding this comment.
Is this a second leak? Or is it related?
There was a problem hiding this comment.
I wouldn't say its a leak as shutdown() will be called in the finalize() as well. It just release memory as fast as possible as you never know when the finalize() method will be run at the end.
There was a problem hiding this comment.
@nmittler so it's more an improvement to free up memory asap
There was a problem hiding this comment.
@nmittler also it's in the modification section but I can also adjust the commit message to make it more explicit if you want too.
There was a problem hiding this comment.
@normanmaurer Yeah I noticed it after .... the commit message is fine as-is. SGTM, thanks!
|
LGTM |
1 similar comment
|
LGTM |
…ke is started
Motivation:
We need to ensure we only add the OpenSslEngine to the OpenSslEngineMap when the handshake is started as otherwise we may produce a memory leak when the OpenSslEngine is created but not actually used. This can for example happen if we encounter a connection refused from the remote peer. In this case we will never remove the OpenSslEngine from the OpenSslEngineMap and so it will never be collected (as we hold a reference). This has as affect that the finalizer will never be run as well.
Modifications:
Result:
No more memory leak with OpenSslEngine if connection is refused.