Close inactive connections and requests

[WyriHaximus]

This builds on top of #405 and further builds out #423 by also close connections with inactive requests.

Closes: #423

[mmoreram]

@WyriHaximus is there any chance you can merge this PR? Seems to be tested, and increases so much the memory performance :)

[WyriHaximus]

@WyriHaximus Thanks for the update!

Can you provide a quick update here, what's your plan for this feature with regards to #425? We've briefly discussed (off channel) that this should eventually send a 408 Request Timeout response for the initial connection but not for keep-alive connections that send no following request. I agree breaking this up into smaller PRs can be useful, but it looks like this might also work in a single PR, so I'll leave this up to you. Let me know if there's anything I can help with and/or if you want me to take over from here +1

@clue Right, been thinking about this, and doesn't it make sense to do this with all requests in flight, instead of only on the first?

[WyriHaximus]

@WyriHaximus is there any chance you can merge this PR? Seems to be tested, and increases so much the memory performance :)

@mmoreram There are a few details we need to polish before this PR is done and it can be merged. Like the 408 responses for when an inflight request times out. And ensuring we don't cut off websocket connections with this.

[WyriHaximus]

@WyriHaximus Thanks for the update! Can you provide a quick update here, what's your plan for this feature with regards to #425? We've briefly discussed (off channel) that this should eventually send a 408 Request Timeout response for the initial connection but not for keep-alive connections that send no following request. I agree breaking this up into smaller PRs can be useful, but it looks like this might also work in a single PR, so I'll leave this up to you. Let me know if there's anything I can help with and/or if you want me to take over from here +1

@clue Right, been thinking about this, and doesn't it make sense to do this with all requests in flight, instead of only on the first?

Just made this change, the code is cleaner as well. I Will check the relevant RFC's later today, but until that my position is that any request in a keep alive connection can timeout for any number of reasons.

[clue]

@WyriHaximus Thanks for the update! Can you provide a quick update here, what's your plan for this feature with regards to #425? We've briefly discussed (off channel) that this should eventually send a 408 Request Timeout response for the initial connection but not for keep-alive connections that send no following request. I agree breaking this up into smaller PRs can be useful, but it looks like this might also work in a single PR, so I'll leave this up to you. Let me know if there's anything I can help with and/or if you want me to take over from here +1

@clue Right, been thinking about this, and doesn't it make sense to do this with all requests in flight, instead of only on the first?

Just made this change, the code is cleaner as well. I Will check the relevant RFC's later today, but until that my position is that any request in a keep alive connection can timeout for any number of reasons.

Did take a look at the relevant RFCs (https://datatracker.ietf.org/doc/html/rfc7230#section-6 and https://datatracker.ietf.org/doc/html/rfc7231#section-6.5.7 come to mind) and it looks like this is indeed under-specified behavior. MDN mentions "Note: some servers merely shut down the connection without sending this message." (https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/408) and this actually seems to match my observations.

• If you open a connection github.com:80 or google.com:80 and don't send a complete request, the connection will be closed with no message after a timeout. Same if you send a request using keep-alive and then wait for the timeout. Same if you send a request using keep-alive and then send an incomplete request header.

• If you open a connection to Apache and don't send any data, the connection will be closed with no message after a timeout. If you open a connection and send an incomplete request header, the connection will be closed with a message after a timeout. In either case, a 408 will be logged. If you send a request using keep-alive and then wait for the timeout, the connection will be closed with no message after a timeout and no log message. If you send a request using keep-alive and then send an incomplete request header, the connection will be closed with an message after a timeout and a log message.

• If you open a connection to nginx, it will close the connection with no message after after a timeout in either case. Even though I'm using default settings (http://nginx.org/en/docs/http/ngx_http_core_module.html#client_header_timeout), I've never seen an 408 response in my tests.

• Note there's also https://www.haproxy.com/blog/haproxy-and-http-errors-408-in-chrome/ which suggests sending a 408 used to be(?) a problem in chrome to its inherent race condition. See also https://stackoverflow.com/questions/42631273/how-do-browsers-handle-http-keepalive-race-condition for some background.

Neither behavior appears to be against specs, so I wonder what behavior we should use here.

[WyriHaximus]

Neither behavior appears to be against specs, so I wonder what behavior we should use here.

@clue For now, merge this as is and I'll do a follow up PR to make the behavior configurable.