Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

safe retry on reset #10007

Closed
rgs1 opened this issue Feb 11, 2020 · 6 comments · Fixed by #35074
Closed

safe retry on reset #10007

rgs1 opened this issue Feb 11, 2020 · 6 comments · Fixed by #35074
Assignees
Labels
area/http design proposal Needs design doc/proposal before implementation

Comments

@rgs1
Copy link
Member

rgs1 commented Feb 11, 2020

We currently support retrying after an upstream reset, but this could happen after the request has been seen -- partially or totally -- by the upstream:

https://github.com/envoyproxy/envoy/blob/master/source/common/router/router.cc#L966

at which point, it might be unsafe to retry. Note only downstream_response_started_ is checked for, we are not checking if the request was sent out already.

Would something like reset-before-request make sense? We currently have this issue, where we have an upstream that closes idle connections just before we start sending the request. But it can't currently be retried, because it would only be safe to retry on connect-failure.

cc: @mattklein123 @derekargueta @fishcakez

@rgs1 rgs1 changed the title safe retry-on reset safe retry on reset Feb 11, 2020
@zuercher zuercher added the question Questions that are neither investigations, bugs, nor enhancements label Feb 11, 2020
@mattklein123
Copy link
Member

cc @snowp for guidance/thoughts on retry policies.

@snowp
Copy link
Contributor

snowp commented Feb 13, 2020

Conceptually this makes sense to me, though we'll need to be specific about the exact behavior: for example, if the first attempt was sent and was retried due to response headers, but on the second attempt the request is reset before the second set of headers are sent, should the second request be retried?

This definitely falls into a similar category as #9946 wrt having too many retry policies, but I think this one is probably simply enough that we can support it

@mattklein123 mattklein123 added design proposal Needs design doc/proposal before implementation area/http and removed question Questions that are neither investigations, bugs, nor enhancements labels Feb 14, 2020
@stale
Copy link

stale bot commented Mar 15, 2020

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 15, 2020
@stale
Copy link

stale bot commented Mar 22, 2020

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions.

@stale stale bot closed this as completed Mar 22, 2020
@Stono
Copy link

Stono commented Jun 25, 2024

Sorry to rekindle an old issue but this would really be useful!

See (long) threads like: #14981; where people deal with 503's. Sending local reply with details upstream_reset_before_response_started{connection_termination}

These are "fixed" by a retry policy of reset or gateway-error, however that can be dangerous. All the resets I observe certainly fall into reset-before-request, so feels like this would be a big win.

@keithmattix
Copy link
Contributor

I'd be interested in potentially implementing this

@alyssawilk alyssawilk reopened this Jul 1, 2024
@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 1, 2024
wbpcode pushed a commit that referenced this issue Jul 12, 2024
<!--
!!!ATTENTION!!!

If you are fixing *any* crash or *any* potential security issue, *do
not*
open a pull request in this repo. Please report the issue via emailing
envoy-security@googlegroups.com where the issue will be triaged
appropriately.
Thank you in advance for helping to keep Envoy secure.

!!!ATTENTION!!!

For an explanation of how to fill out the fields, please see the
relevant section
in
[PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/main/PULL_REQUESTS.md)
-->

Fixes #10007
Commit Message: Add new reset-before-request retry API that will only
retry reset connections if the headers haven't been delivered.
Additional Description:
Risk Level: Low
Testing: Unit and e2e
Docs Changes: Add new reset-before-request
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API
Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

---------

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/http design proposal Needs design doc/proposal before implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants