HTTP/1.x version mismatches and Connection header ambiguity

[zarqman]

I've noticed a couple of issues related to the Connection header and the HTTP/1.x version returned in responses.

1. HTTP/1.x response versions don't match the request, causing ambiguity when Connection is a default value.

When an HTTP/1.0 request is sent to an HTTP/1.1 server:

# request:
GET / HTTP/1.0
...

# response:
HTTP/1.1 200 OK
...

The version mismatch can cause the client to misinterpret the Connection header, since a missing Connection header has different implied defaults in HTTP/1.0 vs HTTP/1.1.

In the above example, should the client believe that the absence of Connection indicate the connection is closed (because the request was HTTP/1.0) or kept alive (because the response was HTTP/1.1)?

I looked at building a PR for this, but I'm unsure how to best approach it. My first thought was to validate the incoming request version in protocol-http1's read_request, and then change async-http's each handler to use request.version where it is presently using @version. However, that begs the question of whether such validation should restrict an HTTP10 server to HTTP/1.0 only, and thus needs to be aware of @version.

I also explored always outputting Connection, even when the default value for that HTTP version. This risks incorrectly adding Connection for upgraded and tunneled responses though, so I'm hesitant to make such a change. There might also be a lurking bug here when the request's Connection specifies a non-default value and the response is upgraded or tunneled, but I haven't verified this.

Because of how the HTTPS class processes inbound connections, I think the above issue is limited to http:// and non-alpn https:// connections.

In attempting to test all of this, I ran into another issue.

2. Requests made via async-http seem to always force @persistent=true and write a corresponding Connection header.

In trying to test the above, I was surprised to discover that adding my own Connection header merely resulted in a duplicate header, and I always got the header from write_connection_header as well.

Example:

client = Async::HTTP::Client.new(..., protocol: HTTP10)
headers = {'connection'=>'close'}
client.get('/', headers)

# what's actually sent in the request:
GET / HTTP/1.0
Connection: close         # from headers above
Connection: keep-alive    # because requests set @persistent=true  

On its own, I'm not sure if this is worth fixing or not--I can easily see it cascading into a far reaching change. It does make it hard to test or otherwise control the Connection header though. Perhaps a facility to just set @persistent for new requests would be more appropriate.

Related, the latest HTTP 1.1 RFC recommends not using keep-alive for HTTP 1.0 clients unless it is known that the remote understands keep-alive, particularly because if the remote is a proxy and doesn't understand keep-alive, it can result in a hung connection. That might suggest that a protocol: HTTP10 client should perhaps default to @persistent=false.

The example above also exposes a minor bug where Async::HTTP::Server's persistent? is confused by the double header and will see keep-alive, disregard close, and keep the connection open. I've submitted a PR against protocol-http for this one.