HTTPServer pass no_keep_alive option to conn params

[protonpopsicle]

regarding: #641

Currently passing no_keep_alive=True to HTTPServer.init while also including "Connection: Close" header in responses does not terminate connections server-side. It seems for the intended behavior to work, no_keep_alive options needs to be passed through to the server's HTTP1ConnectionParameters.

[ploxiln]

As further clarification, the request handler should still add the Connection: close header itself. But even when it does that, the HTTPServer does not close the connection after sending the response, instead it waits for the client to do so (which it most likely will do). Since HTTPServer already has this option, might as well hook it up, and make it do what little it can do.

[ploxiln]

Side question: would it make sense to remove the self.no_keep_alive = no_keep_alive line just above? Is that an "api detail" that anyone might have been depending on?

[bdarnell]

Thanks for the fix for this (untested) feature; looks like it's been broken since the http1connection refactoring. Since it's there we might as well fix it, although it's been broken so long there's little evidence that anyone actually cares (so maybe we should just drop it in a future release).

The http1connection refactoring also means that we're able to inject the Connection: close header into all responses when in no_keep_alive mode if we want to improve this feature rather than get rid of it.

instead it waits for the client to do so (which it most likely will do).

That's deliberate. In TCP-based protocols it is encouraged to initiate the close on the client side because the side that initiates the close will have a TIME_WAIT socket lingering for a few minutes.

Removing the self.no_keep_alive attribute would be fine, I think.

[vijoshi]

@bdarnell following on from this:

The http1connection refactoring also means that we're able to inject the Connection: close header into all responses when in no_keep_alive mode

and this from https://tools.ietf.org/html/rfc2616#section-8.1.2

A significant difference between HTTP/1.1 and earlier versions of
HTTP is that persistent connections are the default behavior of any
HTTP connection. That is, unless otherwise indicated, the client
SHOULD assume that the server will maintain a persistent connection,
even after error responses from the server.

shouldn't Tornado include Connection: close every time it's going to close the connection, regardless of no_keep_alive to meet the standard? Curious, if this is possible after the refactoring you referred to? We are in a situation where proxies that pool HTTP connections and rely on this response header incorrectly assume Tornado is keeping the connection open when it's not. Landed here as I was looking for known issues with Tornado on this. I'd be open to give such a fix a shot if the current design would allow this.

[ploxiln]

The default behavior for tornado HTTPServer is to not close the connections. Does your tornado-using application specify no_keep_alive=True but not set the Connection header? Or is something else happening?

[vijoshi]

The tornado application is the Jupyter Kernel Gateway (https://github.com/jupyter/kernel_gateway). It does not set no_keep_alive. It is being run behind a http proxy that takes care of authentication. The http proxy relies on the Connection: headers returned in the responses from the proxied service (in this case the Jupyter Kernel Gateway) to determine if the http service is keeping the http connection open. A missing Connection: close indicates to the proxy the connection is being kept open (per https://tools.ietf.org/html/rfc2616#section-8.1.2 this looks to be a fair assumption).

So when, the client makes a request with Connection: close. The proxy passes the header through to the Tornado server (the Jupyter service). I believe Tornado honors the Connection: close header by closing the http connection. However, it does not indicate so by including a Connection: close header in it's response. The proxy believes the Tornado service is keeping the connection open and so caches the HTTP connection for reuse. But the reuse will obviously fail. So I think any HTTP proxies working this way would hit this problem with Tornado based apps.

[ploxiln]

I see - yes, tornado does close the connection when it receives the "Connection: close" header.

But it seems like the proxy should realize it sent the request with that header, and you could configure it to not send that header. Quoting from https://tools.ietf.org/html/rfc2616#section-14.10

The Connection general-header field allows the sender to specify options that are desired for that particular connection and MUST NOT be communicated by proxies over further connections.

...

For example, Connection: close in either the request or the response header fields indicates that the connection SHOULD NOT be considered 'persistent' (section 8.1) after the current request/response is complete.

That said, I can see why there might possibly be some value in a tornado HTTPServer option to always wait for the other side to close first.

[bdarnell]

RFC 2616 has been replaced by RFC 7230, which gives more clarity around the Connection: close field in section 6.6:

A client that sends a "close" connection option MUST NOT send further
requests on that connection (after the one containing "close") and
MUST close the connection after reading the final response message
corresponding to this request.

A server that receives a "close" connection option MUST initiate a
close of the connection (see below) after it sends the final response
to the request that contained "close". The server SHOULD send a
"close" connection option in its final response on that connection.
The server MUST NOT process any further requests received on that
connection.

So the kernel gateway is responsible for remembering that it has sent Connection: close and MUST NOT reuse the connection no matter what the server does. Tornado SHOULD add the Connection: close header to the response whenever it is going to close the connection. There's also a more complicated shutdown procedure that is recommended for server-initiated closes:

To avoid the TCP reset problem, servers typically close a connection
in stages. First, the server performs a half-close by closing only
the write side of the read/write connection. The server then
continues to read from the connection until it receives a
corresponding close by the client, or until the server is reasonably
certain that its own TCP stack has received the client's
acknowledgement of the packet(s) containing the server's last
response. Finally, the server fully closes the connection.