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

Only call handlerRemoved(...) if handlerAdded(...) was called during … #8684

Merged
merged 1 commit into from Jan 14, 2019

Conversation

@normanmaurer
Copy link
Member

normanmaurer commented Dec 22, 2018

…adding the handler to the pipeline.

Motivation:

Due a race in DefaultChannelPipeline / AbstractChannelHandlerContext it was possible to have only handlerRemoved(...) called during tearing down the pipeline, even when handlerAdded(...) was never called. We need to ensure we either call both of none to guarantee a proper lifecycle of the handler.

Modifications:

  • Enforce handlerAdded(...) / handlerRemoved(...) semantics / ordering
  • Add unit test.

Result:

Fixes #8676 / #6536 .

Only call handlerRemoved(...) if handlerAdded(...) was called during …
…adding the handler to the pipeline.

Motivation:

Due a race in DefaultChannelPipeline / AbstractChannelHandlerContext it was possible to have only handlerRemoved(...) called during tearing down the pipeline, even when handlerAdded(...) was never called. We need to ensure we either call both of none to guarantee a proper lifecycle of the handler.

Modifications:

- Enforce handlerAdded(...) / handlerRemoved(...) semantics / ordering
- Add unit test.

Result:

Fixes #8676 / #6536 .
@bryce-anderson
Copy link
Contributor

bryce-anderson left a comment

lgtm, but not super familiar with this code.

}
} finally {
// Mark the handler as removed in any case.
setRemoved();

This comment has been minimized.

@bryce-anderson

bryce-anderson Jan 4, 2019

Contributor

Not a part of this change, but it's surprising to me that we call setRemoved() after we call handlerRemoved. For example, if the handler decides to call ctx.isRemoved() in it's handlerRemoved method it will report false which seems inconsistent.

This comment has been minimized.

@normanmaurer

normanmaurer Jan 14, 2019

Author Member

@bryce-anderson let me open another issue to investigate this.

@normanmaurer normanmaurer merged commit 250e249 into 4.1 Jan 14, 2019

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 pipeline_race_fix branch Jan 14, 2019

@normanmaurer normanmaurer added the defect label Jan 14, 2019

normanmaurer added a commit that referenced this pull request Jan 14, 2019

Only call handlerRemoved(...) if handlerAdded(...) was called during …
…adding the handler to the pipeline. (#8684)

Motivation:

Due a race in DefaultChannelPipeline / AbstractChannelHandlerContext it was possible to have only handlerRemoved(...) called during tearing down the pipeline, even when handlerAdded(...) was never called. We need to ensure we either call both of none to guarantee a proper lifecycle of the handler.

Modifications:

- Enforce handlerAdded(...) / handlerRemoved(...) semantics / ordering
- Add unit test.

Result:

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