http upgrade after one or more requests on same connection

[tanner0101]

Expected behavior

An HTTP upgrade request sent immediately after one or more non-upgrade HTTP requests over the same connection will succeed.

Actual behavior

HTTP upgrade requests sent after one or more non-upgrade HTTP requests will not be handled.

Steps to reproduce

1. Open a connection to a NIO HTTP server with an HTTP protocol upgrader configured.
2. Send a keep-alive HTTP request with no upgrade header to the server.
3. On the same connection, send an HTTP request with upgrade headers.
4. Notice that the HTTP protocol upgrader is not invoked.

If possible, minimal yet complete reproducer code (or URL to code)

NIO's HTTPUpgradeHandler will remove itself from the pipeline if it sees an HTTP request with no upgrade header.

SwiftNIO version/commit hash

Latest version, 1.12.0.

Swift & OS version (output of swift --version && uname -a)

Apple Swift version 4.2.1 (swiftlang-1000.11.42 clang-1000.11.45.1)
Target: x86_64-apple-darwin18.2.0
Darwin Agatha.home 18.2.0 Darwin Kernel Version 18.2.0: Fri Oct  5 19:41:49 PDT 2018; root:xnu-4903.221.2~2/RELEASE_X86_64 x86_64

Discussion

I'm posting this issue after a rather involved bug hunt relating to intermittent WebSocket failures in production. It turns out that our reverse proxy was keeping connections to our load balancer alive (server-to-server keep-alive). Because of this, WebSocket upgrade requests sent to our server could be sent over a connection previously used to handle normal HTTP requests. Even though an individual client would never send a normal HTTP request before attempting to upgrade, the server-to-server keep-alive was allowing multiple clients to share a single connection (NIO channel).

Disabling the server-to-server keep-alive ultimately resolved the issue. However, after discussing this with @weissi I wonder if NIO could/should be handling this use case better. It doesn't seem outside the realm of possibility for a client to send one or more normal HTTP requests before attempting to upgrade. The problem of server-to-server keep alive is also likely to affect some people.

I imagine the reason for the HTTPUpgradeHandler removing itself from the pipeline is that there's a non-trivial amount of overhead required in checking for upgrade. Always checking for upgrade could have a large impact on HTTP performance where protocol upgraders are configured. One idea I have here would be to use http_parser->upgrade to check if an upgrade is required instead of searching the HTTPHeaders data.

Thoughts?

[Lukasa]

I imagine the reason for the HTTPUpgradeHandler removing itself from the pipeline is that there's a non-trivial amount of overhead required in checking for upgrade. Always checking for upgrade could have a large impact on HTTP performance where protocol upgraders are configured. One idea I have here would be to use http_parser->upgrade to check if an upgrade is required instead of searching the HTTPHeaders data.

The performance cost was not actually my major concern when writing the implementation, my primary concern was with correctness. Specifically, to support the case where the user does not use the HTTPServerPipelineHandler we need to bring a substantial state machine into the handler in order for it to ensure that it applies the upgrade at the correct time in the face of pipelined requests. When you only allow upgrade on the first request it's a lot easier to be confident that this will not mishandle HTTP pipelining.

I'm open to supporting this, but we need to decide if we're supporting running in pipelined mode for this model. I think we may have to, which incurs a substantial amount of code complexity.

[michal-tomlein]

We ran into this when deploying a Vapor 3 application behind an AWS Application Load Balancer. What made it worse is that neither the ALB nor Vapor allow opting out of keep-alive, so we’ve had to fork vapor/http.

Is this still an issue with NIO 2 and Vapor 4?

[Lukasa]

So far as I know NIO has not changed here.

[michal-tomlein]

Thanks, Cory. I see the issue was added then removed from the 2.0.0 milestone. Is it simply a matter of priorities, or has your thinking changed on whether this is something you’re interested in supporting?

I’m trying to decide whether to pursue a more sophisticated solution than disabling ALB-to-Vapor keep-alive.

[Lukasa]

I think this was just a matter of priorities: while we would like to see this changed, it's not high up our priority list, so it'll probably have to get tackled by a non-core contributor. We're happy to review patches and provide guidance on how to get this fixed though!