HTTP1 conneciton pool attach pending request to half-closed connection #2715
Comments
|
So, the solution is just to check if the remote is closed before attaching a new request? Are you working on a fix @lizan ? |
|
thanks @lizan! |
|
Yeah I believe the solution is make I'm pretty busy today but can start working on a fix tomorrow. |
|
@lizan we can look at a fix here but something about this doesn't make sense to me. Is the upstream HTTP/1.0 or is it not setting |
|
@mattklein123 upstream is free to close the connection in between requests (e.g. due to timeout), even if it previously sent |
|
@PiotrSikora yes, that is true, but if due to timeout there is an inherent race condition here. This is why @ramaraochavali is adding the idle timeout feature. I don't really see how looking for remote close is a viable fix in all cases. We can potentially do it, but there are still races that will be lost. The "correct" solution here is to make sure upstream is setting |
|
I want to point out another scenario which would probably not be solved by just not putting back dead sockets in the pool (that I fixed in fortio while we investigated this): the socket can die afterwards for any reason, so when envoy picks up a connection from the pool that isn't fresh, it needs to auto reconnect if it gets any error before reading anything on the socket, and not attribute that unexpected EOF to the current request and instead just reconnect does that makes sense? |
|
I agree that upstream is not setting Though, the codec_client_->remoteClosed() is returning false (checked here) while the connection already read end of stream is a churn from the half-close support, which should be fixed anyway. |
|
re upstream bug: I don't think the spec forces http1.1 servers to keep connection alive indefinitely or forbid them from changing their mind and closing a connection after they sent the headers/payload ? and again, even if that way the case, it just highlights what may happen anyway with many simple services |
Yes, if there is an obvious fix here, sure, do it, but I'm just explaining this is a bandaid that will not fix all cases of it.
For HTTP/1.1, the spec does say that connections are kept alive until the other side sets |
|
@mattklein123 can you also comment on
|
|
The spec (end of 6.1 of RFC7230) only says:
And in 6.5 it says:
|
|
re upstream bug: also "A client, server, or proxy MAY close the transport connection at any time. " |
|
As I already explained, regardless of what the spec says, in the real world this is a bug. The upstream should be setting
Yes socket can die, timeout, etc. This is what router level retries are for. I don't think handling this in the connection pool makes sense from a complexity perspective. |
|
@mattklein123 sorry, but what you're suggesting basically requires upstream to commit to indefinitely waiting and accepting another request, which doesn't make any sense. Web servers close idle connections all the time. This is known behavior, and there is no requirement to do otherwise. That's not an upstream bug, but a bug in Envoy, since it's queues requests on already closed connection. |
Negative. That is not what is happening here. Upstream is immediately closing the connection. I understand servers can close for any reason, but this is a timing issue. Even if you "fix" this case, there are interleavings in which we will not get the close notification until too late and we will still hit this issue. HTTP/1.1 has this inherent timing issue. As I already explained, this is solved in practice by a) setting |
|
how would you know what the upstream timeout is ? whatever number you pick, it's valid for the upstream server to use 1/2 of that - or again - to close whenever for any reason we're trying to make the sidecar make legacy services better, if a service is doing something allowed by the spec (and probably even if it wasn't) our goal is to do better, not generate spurious errors |
You can't really. This is just a problem with HTTP/1.1 flat out. As I said before, if there is a logic issue with the remote close logic we should absolutely fix that, but it will not fix all cases. There will be interleavings and timings that will be lost and you will hit this error. This is in general what router level retries are for. |
|
I disagree - we can do what I mentioned above: if you get a read error before reading anything else (without reading headers/reply for what you requested), it just means the socket is dead and you need a new one |
You are implementing retry logic now in 2 places. This does not make sense. Use router level retries. If a new retry policy is needed to cover this case we can look into that. |
|
@mattklein123 I agree that upstream that wants to close connection immediately should send Furthermore, the case discussed here (i.e. connection immediately closed, which Envoy detects in the Lastly, I don't see how "setting the idle timeout to less than upstream idle timeout" helps with anything. It just hides the problem, and requires you to know internal settings of upstream, which might not be under your control anyway. |
We are going in circles here. I agree. If Envoy knows the remote is closed, we should not reuse it. If that regressed somehow let's fix it. I'm just explaining that this will not fix all cases and a retry policy is typically needed here to get to 100% SR in all cases. I think we are on the same page? |
|
almost, there is subtle difference though between: the upstream does fail to fulfil a request and we retry it vs envoy fails to realize the socket has been closed per spec and fixes it through a higher level retry. maybe practically we can swallow the error - but a POST for instance shouldn't be retried; yet a POST in a dead socket is an envoy error and should be avoided instead of propagated back to the origin |
If Envoy knows the socket is dead at the time we use it, it's an Envoy error. As I've already said many times, if this has regressed, we should fix it. What I'm explaining is that this is a timing issue and the race will be lost sometimes. This must be handled by higher level retries. We will not add retry capability to the connection pool. Retries are handled by the router. If we need a new retry policy to handle this case we can add it. (Though I don't think we do). |
|
Sorry, maybe I'm misunderstanding. Matt, are you against the connection pool being registered for EPOILLERR/EPOLLIN for currently-unused connections? I'd think if we get a FIN on an idle connection we'd want to close it and prefetch another. Waiting until the connection is assigned to a client request seems like unnecessary delay to me as well. Yes, it's totally possible that we'd get that FIN right after the connection was assigned so retry semantics are required there, but making sure you have useful prefetched connections seems reasonable as well. |
Unfortunately in that case you still have no idea whether the socket was closed after the remote had received and started processing your request (but before it sent a response), so you can't just blindly open a new connection and retry. There's not much that can be done about it other than improving the detection of dead sockets before attempting to write a request to them (I've seen some libraries do a non-blocking read before writing a request to try to detect half-closed sockets.) Even best case that still leaves a window for a race where the request and the FIN cross each other in flight. The common case where this happens in normal operations is keepalive timeouts, and in a controlled environment the new idle timeout can be configured to avoid the race entirely. For the less common case (actual errors or where you have no idea what the server's keepalive timeout is) you'll need to decide whether or not you can accept a retry policy. |
|
you can safely retry HEAD/GET/PUT etc... and for POST you can request 100 continue first the case where the socket has been closed (idle or otherwise) is more common/interesting than the case where the remote side just crashed because of the request you made also, yes, as @alyssawilk mentioned if you do get the eof event before you sent anything, the connection should be redone (the http1.1. socket shouldn't become readable before you send anything on it) In short there are many ways to improve the current state with various degrees of corner case coverage, the fact that there is some 0.001% race condition that may stay shouldn't remove doing the right thing for the most common 99.999% |
@alyssawilk no, I'm not against this. The code used to do this so I think there might have been some regression with the half close work. @lizan will fix. What I was explaining is that even if we do this there is no guarantee this will work even in the immediate close case. Depending on the interleavings on either the host Envoy is running on our depending on how the remote server is implemented, it is very possible that Envoy will read a complete response, get EAGAIN fro the kernel, complete dispatch, assign the idle connection, and then get FIN, causing the issue. So as I think we all agree now (I hope) we should fix any regression with detecting FIN on the connection, but retry will be required to get to 100% in all cases. The upstream server should be setting
@ldemailly as I've now said at least 5 times, if there is a regression it will be fixed. We will not add retry support to the connection pool however. Use router retry. If one of the policies there don't work for you please open a new issue on what policy would make sense. Thanks! |
|
it should be fixed whether it is a regression or a newly discovered issue, not sure why that would matter |
Attach pending upstream requests in next event after onResponseComplete. Risk Level: Medium Testing: unit test, integration test Docs Changes: N/A Release Notes: N/A Fixes #2715 Signed-off-by: Lizan Zhou <zlizan@google.com>
Attach pending upstream requests in next event after onResponseComplete. Risk Level: Medium Testing: unit test, integration test Docs Changes: N/A Release Notes: N/A Fixes envoyproxy#2715 Signed-off-by: Lizan Zhou <zlizan@google.com> Signed-off-by: Rama <rama.rao@salesforce.com>
Description:
The HTTP1 connection pool attach pending request when a response is complete. Though the upstream server may already closed the connection, this will result the pending request attached to it end up with 503.
A sample upstream HTTP1 trace log:
The first request was processed correctly while the 2nd request is attached at
The write never complete and the request was reset immediately.
Repro steps:
It is a little bit hard to reproduce, I'll update when I have a minimal reproducible environment.
The current repro is with Istio load testing tool at 100 qps with mTLS enabled.
See istio/istio#3295 for more details.
The text was updated successfully, but these errors were encountered: