fix(client): retry http1 connection if closed by server

[joelwurtz]

When using client with a http1 server, if connection is closed by server we should retry to acquire a new connection instead of throwing an error.

Add a test to verify this behavior and fix the code, i had to clone the connect url in order for this to work, but maybe there is a better way ?

[fakeshadow]

Retrying should be explicitly issued by API caller. We can't decide for user due to lack of context. For example server may be rejecting a client due to ratelimit and keep trying you may get a ban.

[joelwurtz]

I agree on that, but here it's more that the connection is closed by server, so retry is safe (we have an eof during reading headers).

If you look at the test i do the following :

• do a request on the server (ok)
• restart the server (connection is closed)
• do a new request to the server (should be ok since it's up, but connection was closed so we should create a new one)
• without my patch, the test is failing (and it should not in my point of view, as user should not care if connection was closed or not)

But maybe there is a better way to detect that connection was closed by server instead of having an Eof when reading them ?

Maybe there is something to do when writing request header that is not done actually ?

[fakeshadow]

If you need this you can just write a middleware for it and opt-in. The core logic must not do it for you by default.

[joelwurtz]

But then having a closed (not useable) connection in the pool is not a bug ?

For me it should be in the core and not with a middleware, but will try to change the code to do this as a middleware then

[fakeshadow]

If a broken connection is returned to pool it's a bug. It should be fixed by proper means. Not by retrying implicitly.

[fakeshadow]

The proper way should be recording the keep-alive duration a server promised if there is any and return an error describing it if the server fail to uphold it. Then user can decide what to do with given information. If they need a retry they can opt-in with a middleware.

It also worth mention that an API for keep-alive config of connection pool will also be helpful for situation like this. Or at least give it some tweak as the current default value is around an hour which is not a widely used default among regular seen http servers

[joelwurtz]

I changed the pr to be a middleware instead, so user can opt in to retry those connection if an UnexpectedEof occur during reading the response (which means a closed connection)

[fakeshadow]

Looks good over all. Though for http2 and http3 the error handling could need more attention as they have a higher level of internal error handling than raw io error types.

BTW do you mind if we implement connection pool keep alive error handling first and use that error type for branching instead of raw io error?

[joelwurtz]

Sure, i can look into it, mainly this PR was due to the fact that if was very easy to get UnexpectedEof error from it (like doing a first request, waiting 10 sec, refresh the page and got an error)

Maybe sending / reading KeepAlive header will help for those case (in case where the server correctly manage them)

[joelwurtz]

I'm not sure what would be the best thing to do, for me keep alive should be handled at tcp level but there is no way to do that with tokio tcp stream (have to use the libc for that).

From what i have read, KeepAlive header is only for http1.0 (not http1.1 which assume persistent connection by default), and it seems that keep alive settings should be handled by the transport (so tcp in this case)

Do you have something specific in mind in order to handle keep alive correctly here ?

[fakeshadow]

Connection and Keep-Alive are different headers.

HTTP/1.0 needs explict Connection header to enable keep-alive and HTTP/1.1 can opt in it (which most http clients do). Anyway it's not of concern of this issue.

Keep-Alive is usually attached with the timeout and max key value to inform how long the keep-alive duration will be. It's optional.

The idea is to reading the header when it's presented and set connection keep-alive state accordingly. If it's not provided the client local setting are kicked in. No matter what timeout is set when the keep-alive is still in effect and server decide to close connection. It will be treated as error with a message to inform user that server fail to uphold the keep-alive promise. Either it's a server provided or local settings.

And no HTTP keep alive can't depend on transport layer. It must be managed on top of it by the framework to have an acceptable consistency and accuracy.

[joelwurtz]

I'm not so sure about Kepp-Alive header, i have look at some implementations and have done some tests with nginx and apache, and they never set this header. I feel like this would be never used in real cases scenarios.

[fakeshadow]

I don't know about others but we definitely use it. It hints our resource limited front end devices what to expect from an open socket. As for proxies they have no way to know the real keep alive time as the application it proxy for can shutdown a connection at any moment so it make sense they don't set it automatically.

I'll work out a fix for this when I get spare time for it. For retrying a request this fix is completely transparent other than additional error types for branching on when/if the retry should happen

[joelwurtz]

Replaced by #1206 and #1209