Fix OneTimeReadTimeoutHandler behavior #1235
Conversation
- Extend ReadTimeoutHandler for timeout behavior - Remove self from pipeline once a message is read
| ctx.pipeline().remove(this); | ||
| super.channelRead(ctx, msg); |
There was a problem hiding this comment.
Shouldn't it be channelRead first and then remove?
There was a problem hiding this comment.
I don't think so. If a downstream handler removes us first (for example if the channel is released and HandlerRemovingChannelPool removes it), then remove will fail; so we remove the read timeout handler first, then fire the read down the rest of the pipeline.
There was a problem hiding this comment.
Oh I see, yeah.
Is there any reason why super.channelRead is being used? Should we do ctx.fireChannelRead(msg) ?
There was a problem hiding this comment.
Yeah that would work, but technically this is just a specialized ReadTimeoutHandler so I think it makes more sense to just defer to it's channelRead impl.
There was a problem hiding this comment.
Looking at the source code of IdleStateHandler, there's some extra logic there. Probably doesn't matter for now, but I'd lean towards to using ctx.fireChannelRead directly just in case there's some broken change introduced in the future(somehow).
@Override
public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception {
if (readerIdleTimeNanos > 0 || allIdleTimeNanos > 0) {
reading = true;
firstReaderIdleEvent = firstAllIdleEvent = true;
}
ctx.fireChannelRead(msg);
}There was a problem hiding this comment.
I think that's my point as well though. Not calling super.channelRead could also break behavior because extra logic there.
There was a problem hiding this comment.
Hmm, yeah, I guess that makes sense.
Codecov Report
@@ Coverage Diff @@
## master #1235 +/- ##
============================================
- Coverage 59.27% 59.21% -0.07%
Complexity 4661 4661
============================================
Files 750 750
Lines 23292 23288 -4
Branches 1744 1744
============================================
- Hits 13806 13789 -17
- Misses 8789 8804 +15
+ Partials 697 695 -2
Continue to review full report at Codecov.
|
Add S3 Object Lambda customizations
Description
Motivation and Context
Bug fix.
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsLicense