fix(server): fix graceful_shutdown on freshly opened connections

[flumm]

When opening a http1 connection, the keep_alive state is set to Busy,
preventing a graceful_shutdown in that case. With this patch the
keep_alive disabling and closing of idle connections is decoupled and
should fix this issue.

Closes #1885

Although all tests go through (afaict), i am not entirely sure this is desired behaviour, though
we run into an issue where we use the graceful_shutdown for a daemon reload and an
open connection (with no data) can block our reload... (this issue fixes that)

Signed-off-by: Dominik Csapak d.csapak@proxmox.com

[flumm]

seems most of the ci tests time out, and the one test on macos that fails i cannot reproduce here on linux (i have no access to a mac)

[seanmonstar]

Thanks for the PR!

I'm not sure that accepting a connection should immediately put in an idle state, ready to shutdown. We'd usually assume that a new connection means a request is coming soon. It also affects what a parsing error means.

I think the issue this is linking to should instead just be dealt with by a user selecting on a timeout after triggering a graceful shutdown. It may be that the current graceful shutdown API doesn't easily allow for that, I haven't thought too hard about that part.

[flumm]

Thanks for the PR!

I'm not sure that accepting a connection should immediately put in an idle state, ready to shutdown. We'd usually assume that a new connection means a request is coming soon.

ok no problem

It also affects what a parsing error means.

in what way exactly? (just to understand)

I think the issue this is linking to should instead just be dealt with by a user selecting on a timeout after triggering a graceful shutdown. It may be that the current graceful shutdown API doesn't easily allow for that, I haven't thought too hard about that part.

we solved a part of the issue another way, but is there any mechanism to have a timeout on an incoming connection until the service is called? i did not really find an example of this

[misos1]

I'm not sure that accepting a connection should immediately put in an idle state, ready to shutdown. We'd usually assume that a new connection means a request is coming soon.

@seanmonstar It was already a long time ago when I was thinking about this but it is not true that a request can be coming soon on any already idle keep-alive connection? Why would it be ok to close such a connection but not a freshly opened connection on which still did not arrive first request?

I think the issue this is linking to should instead just be dealt with by a user selecting on a timeout after triggering a graceful shutdown.

But would not such timeout and then forcible shutdown cause situations which would not happen during graceful shutdown? For example if the server is sending large data which will take longer than this timeout. If I am not wrong, graceful shutdown should wait until this is done.

It may be that the current graceful shutdown API doesn't easily allow for that, I haven't thought too hard about that part.

Anyway it could be good to have such a timeout in hyper and set it to something longer like in order of minutes. But I do not think that it is good to wait such a long only because there is a speculatively opened fresh connection (seems some browsers do this maybe to speed up things like I mentioned chrome) so maybe would be good to have another and shorter timeout for such connection? (Of course all timeouts should be configurable).