Adding SslCloseCompletionEventHandler to close channels on tls_close #1041
Conversation
spfink
force-pushed
the
finks/ssl-close
branch
2 times, most recently
from
4830d8b to
7e2db6a
Compare
| /** | ||
| * Handlers {@link SslCloseCompletionEvent}s that can are sent whenever an SSL channel | ||
| * goes inactive. This most commonly occurs on a tls_close sent by the server. Channels | ||
| * in this state can't be reused so we must close them. |
There was a problem hiding this comment.
Don't know what I was writing here.
| if (evt instanceof SslCloseCompletionEvent) { | ||
| ctx.close(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Should this fix be pushed upstream to netty?
There was a problem hiding this comment.
As far as I know, doing what we are doing here is standard practice. I'm not sure why Netty doesn't close the channel the SslHandler.
spfink
force-pushed
the
finks/ssl-close
branch
from
7e2db6a to
8ec4eca
Compare
|
@millems, as far as real world verification, yes. I can reproduce the issue reliably on my local machine. This change significantly reduces the number of occurrences if not entirely. It's still possible that S3 (or whoever) closes the connection after you picked it up from the pool and before reading/writing. We need to handle that case as well. |
| * in this state can't be reused so they must be closed. | ||
| */ | ||
| @SdkInternalApi | ||
| public class SslCloseCompletionEventHandler extends ChannelInboundHandlerAdapter { |
There was a problem hiding this comment.
minor, might be worth creating a singleton across channels to reduce GC since this handler is stateless.
nit, final on this class
| public class SslCloseCompletionEventHandler extends ChannelInboundHandlerAdapter { | ||
|
|
||
| @Override | ||
| public void userEventTriggered(ChannelHandlerContext ctx, Object evt) { |
There was a problem hiding this comment.
Do we need to forward the event if it's not a SslCloseCompletionEvent?
Codecov Report
@@ Coverage Diff @@
## master #1041 +/- ##
============================================
+ Coverage 55.7% 55.71% +<.01%
- Complexity 4610 4612 +2
============================================
Files 816 817 +1
Lines 27944 27950 +6
Branches 2251 2251
============================================
+ Hits 15567 15572 +5
Misses 11657 11657
- Partials 720 721 +1
Continue to review full report at Codecov.
|
spfink
force-pushed
the
finks/ssl-close
branch
from
8ec4eca to
46b5e11
Compare
…7a594da51 Pull request: release <- staging/1cf99818-ab00-4966-b661-dae7a594da51
Description
Adds a new class to handle SslCloseCompletionEvents that occur when a SSL channel goes inactive and can no longer be used.
Motivation and Context
Fixes the case where an channel is in the pool but the server has sent a tls_close event for that channel.
Testing
Added test for handler.
Local testing significantly reduces the number of "Server Failed To Send A Complete Response" errors.
Types of changes
Checklist
mvn installsucceedsLicense