Skip to content
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

Remove a race condition from the WebSocket upgrade #217

Merged
merged 1 commit into from
Jul 26, 2019
Merged

Conversation

pushkarnk
Copy link
Contributor

An upgrade to WebSocket invokes three handlers in an order. First, the
shouldUpgrade handler supplied by the user is run to decide if an upgrade
must go through. This handler also supplies additional headers to be sent in
the response. Next, after the WebSocket upgrader is done upgrading the
pipeline, the completionHandler is called. We remove Kitura-NIO's
HTTPRequestHandler here. Next, the upgradePipelineHandler is invoked. This
handler allows us to add all the WebSocket related ChannelHandlers.

For an undocumented reason, we saved the ChannelHandlerContext received by
the completionHandler in the HTTPServer and later used it upgrade the
pipeline in upgradePipelineHandler. This can easily lead to a race condition
where we saved the ChannelHandlerContext for a connection, into the
HTTPServer but before it could be used in upgradePipelineHandler, it was
overwritten by the upgrade happening on another connection. Consequently, we
never upgraded the pipeline of the former connection. This could lead to
different kinds of failures.

The upgradePipelineHandler has Channel as one of its parameters. Hence
there is no need to store the ChannelHandlerContext for use in this closure.
Consequently, we have to use a Channel to initialize an HTTPServerRequest,
which, in turn, is modified to accept a Channel instead of a
ChannelHandlerContext.

An upgrade to WebSocket invokes three handlers in an order. First, the
`shouldUpgrade` handler supplied by the user is run to decide if an upgrade
must go through. This handler also supplies additional headers to be sent in
the response. Next, after the WebSocket upgrader is done upgrading the
pipeline, the `completionHandler` is called. We remove Kitura-NIO's
`HTTPRequestHandler` here. Next, the `upgradePipelineHandler` is invoked. This
handler allows us to add all the WebSocket related `ChannelHandler`s.

For an undocumented reason, we saved the `ChannelHandlerContext` received by
the `completionHandler` in the `HTTPServer` and later used it upgrade the
pipeline in `upgradePipelineHandler`. This can easily lead to a race condition
where we saved the `ChannelHandlerContext` for a connection, into the
`HTTPServer` but before it could be used in `upgradePipelineHandler`, it was
overwritten by the upgrade happening on another connection. Consequently, we
never upgraded the pipeline of the former connection. This could lead to
different kinds of failures.

The `upgradePipelineHandler` has `Channel` as one of its parameters. Hence
there is no need to store the `ChannelHandlerContext` for use in this closure.
Consequently, we have to use a `Channel` to initialize an `HTTPServerRequest`,
which, in turn, is modified to accept a `Channel` instead of a
`ChannelHandlerContext`.
@bridger
Copy link
Contributor

bridger commented Jul 23, 2019

Great find!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants