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

Avoid retries when any data was written to the backend #3285

Conversation

m3co-code
Copy link
Contributor

What does this PR do?

Fix issue where requests are retried even though one backend accepted and may be processing this request already.

Motivation

Right now it is possible that retries happen, even when the backend received the request already. An example scenario:

  • the timeout ResponseHeaderTimeout is configured to be 3 seconds
  • a request is issued against the backend and the backend starts processing it the backend takes overall 5 seconds to process the request
  • due to the timeout, go will abort the requests after 3 seconds and return a network error
  • Traefik would have retried that request as it is only checking for net errors so far

To avoid this, the approch is to use httptrace to disable retries as soon as there was anything written to the backend.

Websocket requests can't be retried at this point in time. This is due to the fact that gorilla/websocket doesn't use the request context and so we don't get httptrace information. Websocket clients
should however retry on their own anyway.

More

  • Added/updated tests

@m3co-code
Copy link
Contributor Author

I'm wondering whether I should change that PR to be against v1.6, as it's basically a bug fix. WDYT?

@juliens
Copy link
Member

juliens commented May 30, 2018

Thanks for this PR!

We think we really need to handle websocket, that's why we are waiting for news on gorilla/websocket#383.

@m3co-code
Copy link
Contributor Author

Thanks for the response @juliens. I still think that websocket clients should be resilient to network failures and therefore, retries are not an important feature for them as they have to retry on their own. That's why I see the bugfix of potentially duplicated requests due to the retry logic more important than retries for websockets. jm2c

@juliens
Copy link
Member

juliens commented Jun 15, 2018

@marco-jantke after more discussion about this, and as my PR on gorilla/websocket seems stuck, we agree that we could merge this one without the websocket support, and we will do another PR as soon as things move on gorilla/websocket.
But as it makes some kind of regression on websocket retry support, we think we will merge it in the next minor release (1.7) and not in the next bugfix release. WDYT?

@m3co-code
Copy link
Contributor Author

SGTM.

@m3co-code m3co-code force-pushed the avoid-retries-when-connection-was-established branch from a7dde98 to b09b517 Compare June 18, 2018 13:50
Right now it is possible that retries happen, even when the backend
received the request already. An example scenario:

- the timeout ResponseHeaderTimeout is configured to be 3 seconds
- a request is issued against the backend and the backend starts
  processing it the backend takes overall 5 seconds to process the reuqest
- due to the timeout, go will abort the requests after 3 seconds and return a network error
- Traefik would have retried that request as it is only checking for net errors so far

To avoid this, the approch is to use httptrace to disable retries as
soon as there was anything written to the backend.

Websocket requests can't be retried at this point in time.
This is due to the fact that gorilla/websocket doesn't use the request
context and so we don't get httptrace information. Websocket clients
should however retry on their own anyway.
@m3co-code m3co-code force-pushed the avoid-retries-when-connection-was-established branch from b09b517 to c952860 Compare June 18, 2018 13:53
@ldez ldez added this to the 1.7 milestone Jun 18, 2018
Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@traefiker traefiker merged commit e31c85a into traefik:master Jun 19, 2018
@m3co-code m3co-code deleted the avoid-retries-when-connection-was-established branch June 19, 2018 13:31
@mmatur mmatur changed the title avoid retries when any data was written to the backend Avoid retries when any data was written to the backend Jul 9, 2018
@juliens juliens mentioned this pull request Aug 29, 2018
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants