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]

Makes me wonder if we should expose the RequestHeaderParser instead, given we only use the timeout value to pass it forward. (Refs #359)

[WyriHaximus]

As in also make it possible to inject a different header parser? We could do that so we can have a C based extension for parser to speed it up?

[clue]

KISS: The StreamingServer is marked @internal, as is the RequestHeaderParser. This means we can easily inject the RequestHeaderParser to the StreamingServer inside the Server class. Constructor may have to be changed, but doesn't affect BC, because the public Server interface is not affected (at least not as part of this change).

[WyriHaximus]

Yes, I'm fully aware, I'm also thinking ahead. But maybe, if we want to go there, we should ship that C based extension to speed it up

[clue]

I'm not sure this is currently our hot path, but if we have evidence an optional extension will speed things up, I'm all for it 👍 I think we're on the same page here and should probably leave this up for a follow-up PR?

In either case I think your changes are a welcome improvement as is and injecting the RequestHeaderParser into the StreamingServer can help simplify the dependency injection both now and longer-term 👍

[WyriHaximus]

I'm not sure this is currently our hot path, but if we have evidence an optional extension will speed things up, I'm all for it 👍 I think we're on the same page here and should probably leave this up for a follow-up PR?

Of course, that would be a follow-up PR. And we have tons of other things to do.

In either case I think your changes are a welcome improvement as is and injecting the RequestHeaderParser into the StreamingServer can help simplify the dependency injection both now and longer-term 👍

Sounds good! 🚢 🇮🇹 !

(Will fix that thing below this now.)

[clue]

It's my understanding the HttpServer should be simplified to call something like new StreamingServer($middleware, new RequestHeaderParser($loop, $idleConnectionTimeout)).

[WyriHaximus]

It took some work, but I think it's worth the refactoring ❤️

[WyriHaximus]

Took some working on this, but ended up with new StreamingServer($middleware, $idleConnectionTimeout) because the connection/requests timing out is handled in the StreamingServer.

[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.

[clue]

It's my understanding the HttpServer should be simplified to call something like new StreamingServer($middleware, new RequestHeaderParser($loop, $idleConnectionTimeout)).