Proper fix for low-latency client streaming #6666
Labels
bug 🐞
Something isn't working
Milestone
Comments
mholt
added
bug 🐞
mholt
added a commit
that referenced
this issue
Nov 12, 2024
…eam mode i.e. Revert commit f5dce84 Two years ago, the patch in #4952 was a seemingly necessary way to fix an issue (sort of an edge case), but it broke other more common use cases (see #6666). Now, as of #6669, it seems like the original issue can no longer be replicated, so we are reverting that patch, because it was incorrect anyway. If it turns out the original issue returns, a more proper patch may be in #6669 (even if used as a baseline for a future fix). A potential future fix could be an opt-in setting.
mholt
added a commit
that referenced
this issue
Nov 12, 2024
…eam mode i.e. Revert commit f5dce84 Two years ago, the patch in #4952 was a seemingly necessary way to fix an issue (sort of an edge case), but it broke other more common use cases (see #6666). Now, as of #6669, it seems like the original issue can no longer be replicated, so we are reverting that patch, because it was incorrect anyway. If it turns out the original issue returns, a more proper patch may be in #6669 (even if used as a baseline for a future fix). A potential future fix could be an opt-in setting.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In #4922 we had a request to leave the connection open to the backend when the client disconnects before the response (instead of immediately closing the upstream connection), so that the backend would finish receiving what the client sent, when the client doesn't care about the response.
This was implemented in #4952 by simply ignoring the context cancellation when connecting to the upstream when
flush_intervalis -1.Obviously, in hindsight, this is flawed. Not only does it overload
flush_intervalin an incorrect and unexpected way, it leaves resources in use.We had a sponsor report that gRPC connections with backends weren't being closed after clients disconnected, due to this patch. (Still being verified but seems quite probable.)
In 8c9e87d (not mainline branch) I separated
flush_intervalfrom a new option,ignore_client_gonethat explicitly enables the behavior of leaving open the connection to the backend.However, a proper fix is to have an option that, instead of just leaving the connection with the backend open until the backend decides it's done, will close the connection with the backend after it has received everything the client sent before it disconnected. If we mainline a change, I'd prefer it to be the proper fix.
The text was updated successfully, but these errors were encountered: