Skip to content

NIOTypedHTTPServerUpgradeHandler: propagate error and remove self on invalid HTTP ordering - #3714

Open
shoemoney wants to merge 1 commit into
apple:mainfrom
shoemoney:fix/typed-upgrade-handler-error-propagation
Open

NIOTypedHTTPServerUpgradeHandler: propagate error and remove self on invalid HTTP ordering#3714
shoemoney wants to merge 1 commit into
apple:mainfrom
shoemoney:fix/typed-upgrade-handler-error-propagation

Conversation

@shoemoney

Copy link
Copy Markdown

Motivation

NIOTypedHTTPServerUpgradeHandler.channelRead(context:requestPart:) handles the .failUpgradePromise(error) action from the state machine like this:

case .failUpgradePromise(let error):
    self.upgradeResultPromise.fail(error)

That's the only effect. It fails the internal upgradeResultFuture, but nothing else happens: the error is never fired down the pipeline, and the handler never removes itself, so it just sits there in the pipeline having silently absorbed a protocol error.

The untyped sibling, HTTPServerUpgradeHandler, handles the equivalent case (an unexpected HTTP part ordering, firstRequestHeadReceived in HTTPServerUpgradeHandler.swift) differently:

guard case .head(let request) = requestPart else {
    context.fireErrorCaught(HTTPServerUpgradeErrors.invalidHTTPOrdering)
    self.notUpgrading(context: context, data: requestPart)
    return
}

notUpgrading(context:data:) ends with:

// Ok, we've delivered all the parts. We can now remove ourselves, which should happen synchronously.
context.pipeline.syncOperations.removeHandler(context: context, promise: nil)

So the untyped handler both fires the error down the pipeline and removes itself; the typed handler does neither.

Modifications

NIOTypedHTTPServerUpgradeHandler's .failUpgradePromise case now also calls context.fireErrorCaught(error) and context.pipeline.syncOperations.removeHandler(self, promise: nil), in the same order already used by the existing .fireErrorCaughtAndRemoveHandler case a few lines below it in the same file.

Added testTypedUpgradeHandlerBarfsOnUnexpectedOrdering, which drives the typed handler directly on an EmbeddedChannel with an out-of-order HTTPServerRequestPart and asserts both that upgradeResultFuture fails with .invalidHTTPOrdering and that the handler removes itself from the pipeline.

Result

The typed upgrade handler now surfaces invalid-ordering errors to the rest of the pipeline and cleans itself up, matching HTTPServerUpgradeHandler's existing behavior.

…invalid HTTP ordering

Motivation:

When channelReadRequestPart() sees an unexpected ordering of HTTP
parts, the state machine returns .failUpgradePromise(error) and the
handler only fails the internal upgradeResultPromise. It never calls
context.fireErrorCaught(error), so nothing downstream in the pipeline
learns that anything went wrong, and it never removes itself from the
pipeline, so it lingers there after the failure. The untyped sibling,
HTTPServerUpgradeHandler, handles the equivalent invalidHTTPOrdering
case in firstRequestHeadReceived() by firing the error down the
pipeline and then removing itself (via notUpgrading()).

Modifications:

In NIOTypedHTTPServerUpgradeHandler.channelRead(context:requestPart:),
the .failUpgradePromise case now also calls context.fireErrorCaught(
error) and context.pipeline.syncOperations.removeHandler(self,
promise: nil), matching the order already used by the
.fireErrorCaughtAndRemoveHandler case elsewhere in the same handler.

Added a test, testTypedUpgradeHandlerBarfsOnUnexpectedOrdering, that
drives the typed handler directly on an EmbeddedChannel with an
out-of-order HTTPServerRequestPart and asserts that both the upgrade
result future fails and the handler removes itself from the pipeline.

Result:

The typed upgrade handler now surfaces invalid-ordering errors to the
rest of the pipeline and cleans itself up, matching the behavior of
the untyped HTTPServerUpgradeHandler.
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.

1 participant