scylladb
Make http client restart requests
There's an issue with maintaining keep-alive sockets by http client.
Some prior art first.
The HTTP client uses seastar connections to send requests over and receive responses back. Typically the connection is a TCP socket (or TLS connection which is a wrapper over TCP socket too). Once request-response cycle is over the connection used for that is kept by a client in a "pool". Making another http request may then pick up the existing connection from the pool thus avoiding the extra latency of establishing new connection. Pool may thus accumulate more than one connection if the client user sends several requests in parallel.
HTTP servers may sometimes want to terminate the connections it keeps. This can happen in one of several ways.
The "gentle" way is when server adds the "connection: close" header to its response. In that case client would handle the response and will not put the connection back to pool, but instead would just close it. So next request would either pick some other connection from pool or would need to establish a new one.
Less gentle way a server may terminate a connection is by closing it, so the underlying TCP stack would communicate regular TCP FIN-s. On the client side there's connected_socket::wait_input_shutdown() method that returns a future that gets resolved when kernel terminates the connection by peers request. In case client's connection is kept in pool it will be closed and removed from the pool behind the scenes. Next request won't even notice that -- it will either pick some other connection from pool, or will establish a new one.
Sometimes more unpleasant situation happens. It can be either a race or deliberate server's "abort". In the former case, server closes the connection and TCP starts communicating FIN-s in parallel with client picking up a connection for its new request. In that case, even if kernel resolves "input-shutdown" event described above, due to seastar event loop and batch flusher, client would see that the connection is closed after it had picked it from pool and had chance to put some data into it. In the latter case server closes the connection in the middle of reading the request from the client or even in the middle of writing back the response. This is very unlikely, but still happens from time to time.
Having said the above, the problem -- when user calls client::make_request() and client steps on the "unpleasant" server-side connection closure, the request making resolves with exception and user has to do something about it. There are two options here -- handle the exception somehow or ask client to make the same request again (restart the request). This PR suggest that the latter choice can be implemented in the HTTP client code. If user doesn't want to restart, it may ask client not to do it, the new API allows for that.
First (a side node) -- to tell the "server closed its socket" from other exceptions the "transport exception" is introduced. This is the system error with EPIPE or ECONABORTED code in it. EPIPE is reported when it comes from writing the request, ECONABORTED is reported when it comes from reading the response.
Next, there's only one way to restart the request -- client should get new connection somehow and repeat the request-response cycle. There are two options where to get new connection from -- from pool (if it's there) or establish a new one. While experimenting with HTTP client I noticed that picking up connection from pool often results in several "transport exception"-s in a row, as if server was closing connections in batches. So this PR makes a shortcut for restarts it always establishes new connection to server (which, after request-response cycle, is put back into pool normally).
And the last, but not least, restart only happens once, because the chance of observing "transport exception" from newly established connection is considered to be low enough not to care. Respectively, if a request is made over new connection (not over a connection from pool) and "transport exception" pops up it's not restarted.
@nyh , @tchaikov this is the restart implementation without magic numbers (but more hairy) as I mentioned here. Mind taking a look. Patches 1 through 5 are identical in both PRs.
I do not yet have strong preference to either approach, both help S3 client recover from synchronously dropped connections
@tchaikov , thank you for the review. I'd also like you to share your thoughts about which restarting mechanism you like more -- this one (that replays the request once over fresh new connection) or the #1847 approach (that implements backoff-retry and that may potentially use other connections from pool)
@nyh , reminder ping. Please tell which approach sounds more reasonable to you -- this or #1847
@tchaikov , thank you for the review. I'd also like you to share your thoughts about which restarting mechanism you like more -- this one (that replays the request once over fresh new connection) or the #1847 approach (that implements backoff-retry and that may potentially use other connections from pool)
i'd prefer retrying with backoff. and i don't have strong preference without good reasoning. i'd go with a retry_strategy class, if i were you. and the retry_strategy would need to determine
- if we should retry given a certain error if we've retried for n times.
- how long shall we wait before the next retry if we've retried n times.
a default implementation of retry_strategy is provided for the backoff and retry policy. so the magic numbers can have a better home without losing the flexibility at the expense of introducing more complexity.
The #1847 has the retry_param to configure the way requests are retried. But anyway, thank you for the feedback.
@nyh , please, let's decide which way to go -- backoff-retry or force-reconnect
@nyh , please, need your feedback on which http client retry approach we adopt -- force-reconnect (this PR) or backoff-retry (#1847 ). Or both :)
upd:
- get rid of force_new_connection boolean
- added comment in front of transport_error exception
upd:
- rebased
- report
bool connection_from_poolin transport exception - rephrased the comment describing retry vs connection limits benign race
- made retry/not-retry configurable (off by default)
upd: Removed the per-connection tracking of number of requests served and simplified the retry logic -- now it retries over new connection regardless of what. Respectively, there's no longer need in dedicated transport_error exception so it's also removed
@nyh , let's try to resurrect this PR. I rebased it, tuned up the description and resolved all the controversial comments from the previous iterations (the last one left is about the approach itself)
@nyh , let's try to resurrect this PR. I rebased it, tuned up the description and resolved all the controversial comments from the previous iterations (the last one left is about the approach itself)
Ok, I'll try to to fairly re-review this PR from scratch. Earlier in https://github.com/scylladb/seastar/pull/1883#discussion_r1426480383 I suggested that a different reviewer should be brought in, with a fresh view, but at this point I don't remember what I said and what I didn't like so maybe my own view will be fresh as well ;-)
Before reading the code, I want to point out that the new commit message was very helpful, and I feel like I finally understand what you're doing. It even makes a lot of sense if the only kind of error you want to handle is the case of the server closing an inactive connection without the client noticing, and in that case you convinced me that what had looked to me like arbitrary rules actually makes sense. Plus I like the fact that there seems to be fewer arbitrary rules here (no "first request on connection" special handling, etc.)
What still bothers me a bit that this is not really the only kind of error that exists in the wild for an HTTP client. In a long request (e.g., some very long upload), there is a non-zero probability that the connection will break in the middle of the connection - not because we missed a closing (the scenario you considered), but really because of a server problem. So if there's a p=0.001% chance that a 1GB transfer will fail in the middle, there is a p^2= 0.000001% chance both the original transfer and its retry will also fail - which is small but not zero, so the decision to retry only once is less justified for this case. But I do agree that if the API can retry once and also respond with a failure and let the client retry even more times, that's fine too. Other APIs also have such limitations - for example the boto3 API for DynamoDB retries a temporary failure, by default, 3 times, without returning to the user. Why 3 and not 7? It doesn't matter. If it fails 3 retries it will return the failure to the user, the the user can try 3 more times if he wants.
Now I'll start reading the code.
upd:
- added tests for incorrect server response and incomplete server response
- added documentation to newly introduced "retry" option
- added test for request retrying on server transport error
The new version looks really good (and well-tested), thanks. I just wrote a few comments for your consideration (I tentatively "approved" it).