New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ChannelInitializer may be invoked multiple times when used with custo… #8620

Merged
merged 1 commit into from Dec 5, 2018

Conversation

Projects
None yet
3 participants
@normanmaurer
Copy link
Member

normanmaurer commented Dec 4, 2018

…m EventExecutor.

Motivation:

The ChannelInitializer may be invoked multipled times when used with a custom EventExecutor as removal operation may be done asynchronously. We need to guard against this.

Modifications:

  • Change Map to Set which is more correct in terms of how we use it.
  • Ensure we only modify the internal Set when the handler was removed yet
  • Add unit test.

Result:

Fixes #8616.

ChannelInitializer may be invoked multiple times when used with custo…
…m EventExecutor.

Motivation:

The ChannelInitializer may be invoked multipled times when used with a custom EventExecutor as removal operation may be done asynchronously. We need to guard against this.

Modifications:

- Change Map to Set which is more correct in terms of how we use it.
- Ensure we only modify the internal Set when the handler was removed yet
- Add unit test.

Result:

Fixes #8616.
@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Dec 4, 2018

@atcurtis This should fix your problem

initMap.remove(ctx);
// The removal may happen in an async fashion if the EventExecutor we use does something funky.
if (ctx.isRemoved()) {
initMap.remove(ctx);

This comment has been minimized.

@ejona86

ejona86 Dec 4, 2018

Member

What's the point of this line? Wouldn't we only get here if handlerRemoved() was called? Is the concern that someone could override handlerRemoved()?

This comment has been minimized.

@normanmaurer

normanmaurer Dec 4, 2018

Author Member

yes :/

initMap.remove(ctx);
} else {
// Ensure we always remove from the Map in all cases to not produce a memory leak.
ctx.channel().closeFuture().addListener(new ChannelFutureListener() {

This comment has been minimized.

@ejona86

ejona86 Dec 4, 2018

Member

This will retain ctx until the channel is closed, even if handlerRemoved() is called. If this is a one-off initializer (not reused) then that could greatly increase the lifetime of some objects used during handshakes and the like.

Is this seriously the only way we can clean up properly? Is the problem that the methods can be overridden? If so, then maybe we should fix that in Netty 5? Or maybe we could avoid the check-for-double-register problem by having an alternative approach to 26aa348 in Netty 5?

It is possible to cancel the future to clean up, but the amount of effort necessary to do something simple makes me question if this is more of systemic problem that should be addressed in Netty 5.

This comment has been minimized.

@normanmaurer

normanmaurer Dec 4, 2018

Author Member

@ejona86 yeah I think its the best we can do for netty 4 to ensure we not leak if the user overrides handlerRemoved(...).Thats also why I check isRemoved() first.

This comment has been minimized.

@trustin

trustin Dec 5, 2018

Member

Perhaps we could check handlerRemoved() was overridden at instantiation time using reflection and take the fast path?

This comment has been minimized.

@normanmaurer

normanmaurer Dec 5, 2018

Author Member

@trustin we could ... that said I think it does not really worth it as ctx.isRemoved() should be true if the EventExecutor is not some funky custom implemention that is is used with ChannelInitializer. So I would prefer to keep things simple for now.

This comment has been minimized.

@trustin

trustin Dec 5, 2018

Member

Agreed.

@trustin

trustin approved these changes Dec 5, 2018

@trustin

This comment has been minimized.

Copy link
Member

trustin commented Dec 5, 2018

You may want to document about this behavior in Javadoc though, e.g. risks of overriding handlerRemoved() and using a certain EventExecutor implementation.

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Dec 5, 2018

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Dec 5, 2018

@trustin actually I think it is fine to not add docs as the same is true for other methods as well.

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Dec 5, 2018

@ejona86 any other comment ?

@ejona86

ejona86 approved these changes Dec 5, 2018

@normanmaurer normanmaurer merged commit 8331248 into 4.1 Dec 5, 2018

4 checks passed

pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details
pull request validation (centos6-java9) Build finished.
Details

@normanmaurer normanmaurer deleted the initializer_multiple_invoke branch Dec 5, 2018

normanmaurer added a commit that referenced this pull request Dec 5, 2018

ChannelInitializer may be invoked multiple times when used with custo…
…m EventExecutor. (#8620)

Motivation:

The ChannelInitializer may be invoked multipled times when used with a custom EventExecutor as removal operation may be done asynchronously. We need to guard against this.

Modifications:

- Change Map to Set which is more correct in terms of how we use it.
- Ensure we only modify the internal Set when the handler was removed yet
- Add unit test.

Result:

Fixes #8616.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment