Skip to content
New issue

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

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

Already on GitHub? # to your account

Make pipeline handler behave better when removed #1080

Merged

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Jul 23, 2019

Motivation:

The HTTPServerPipelineHandler is removed from pipelines in many cases,
but the most common case is over upgrade. While the handler is
removable, it doesn't make any effort to ensure that it leaves the
pipeline in a sensible state, which is pretty awkward.

In particular, there are 3 things the pipeline handler may be holding
on to that can lead to damage. The first is pipelined requests: if there
are any, they should be delivered, as the user may be deliberately
allowing pipelining.

The second thing is read() calls. The HTTPServerPipelineHandler exerts
backpressure on clients that aggressively pipeline by refusing to read
from the socket. If that happens, and then the handler is removed from
the channel, it will "forget" to restart reading from the socket on the
way out. That leaves the channel quietly in a state where no reads will
occur ever again, which is pretty uncool.

The third thing is quiescing. The HTTPServerPipelineHandler catches
quiescing events and allows them to deliver a response before closing a
connection. If that has happened when the pipeline handler is removed,
it should fall back to the behaviour as though it were not there.

Modifications:

  • Added a handlerRemoved implementation to play event state that should
    be replayed.
  • Added a channelInactive implementation to drop data.

Result:

More graceful handler removal.

@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Jul 23, 2019
@Lukasa Lukasa added this to the 2.6.0 milestone Jul 23, 2019
@Lukasa Lukasa requested a review from weissi July 23, 2019 12:19
@Lukasa Lukasa force-pushed the cb-pipeline-handler-mustnt-freeze-reading-forever branch from 84b9884 to 5137cdc Compare July 23, 2019 12:38
Motivation:

The HTTPServerPipelineHandler is removed from pipelines in many cases,
but the most common case is over upgrade. While the handler is
removable, it doesn't make any effort to ensure that it leaves the
pipeline in a sensible state, which is pretty awkward.

In particular, there are 3 things the pipeline handler may be holding
on to that can lead to damage. The first is pipelined requests: if there
are any, they should be delivered, as the user may be deliberately
allowing pipelining.

The second thing is read() calls. The HTTPServerPipelineHandler exerts
backpressure on clients that aggressively pipeline by refusing to read
from the socket. If that happens, and then the handler is removed from
the channel, it will "forget" to restart reading from the socket on the
way out. That leaves the channel quietly in a state where no reads will
occur ever again, which is pretty uncool.

The third thing is quiescing. The HTTPServerPipelineHandler catches
quiescing events and allows them to deliver a response before closing a
connection. If that has happened when the pipeline handler is removed,
it should fall back to the behaviour as though it were not there.

Modifications:

- Added a handlerRemoved implementation to play event state that should
    be replayed.
- Added a channelInactive implementation to drop data.

Result:

More graceful handler removal.
@Lukasa Lukasa force-pushed the cb-pipeline-handler-mustnt-freeze-reading-forever branch from 5137cdc to b183c70 Compare July 23, 2019 16:43
@Lukasa Lukasa requested a review from weissi July 23, 2019 16:44
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, lgtm!

@weissi weissi merged commit 6a79d46 into apple:master Jul 23, 2019
@weissi weissi modified the milestones: 2.6.0, 2.5.1 Jul 23, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants