More correct fix for using ChannelInitializer with custom EventExecutor. #8633
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: 8331248 did make some changes to fix a race in ChannelInitializer when using with a custom EventExecutor. Unfortunally these where a bit racy and so the testcase failed sometimes. Modifications: - More correct fix when using a custom EventExecutor - Adjust the testcase to be more correct. Result: Proper fix for #8616.
ejona86
reviewed
Dec 6, 2018
| ChannelPipeline pipeline = ctx.pipeline(); | ||
| if (pipeline.context(this) != null) { | ||
| pipeline.remove(this); | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| private void remove(final ChannelHandlerContext ctx) { |
There was a problem hiding this comment.
This is a confusing name now. Call it removeState or clearState?
| @@ -79,6 +79,9 @@ public final void channelRegistered(ChannelHandlerContext ctx) throws Exception | |||
| // we called initChannel(...) so we need to call now pipeline.fireChannelRegistered() to ensure we not | |||
| // miss an event. | |||
| ctx.pipeline().fireChannelRegistered(); | |||
|
|
|||
| // We are done with init the Channel, removing the initializer now. | |||
There was a problem hiding this comment.
This comment seems to be wrong. The initializer was already removed in initChannel.
|
@ejona86 addressed |
ejona86
approved these changes
Dec 7, 2018
normanmaurer
added a commit
that referenced
this pull request
Dec 7, 2018
…or. (#8633) Motivation: 8331248 did make some changes to fix a race in ChannelInitializer when using with a custom EventExecutor. Unfortunally these where a bit racy and so the testcase failed sometimes. Modifications: - More correct fix when using a custom EventExecutor - Adjust the testcase to be more correct. Result: Proper fix for #8616.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
8331248 did make some changes to fix a race in ChannelInitializer when using with a custom EventExecutor. Unfortunally these where a bit racy and so the testcase failed sometimes.
Modifications:
Result:
Proper fix for #8616.