proxy: set tcp write timeout to max timeout timeout value or default #163
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jphines
force-pushed
the
tcp-write-timeout-set-to-max-upstream-timeout
branch
from
57fb212
to
b390f3c
Compare
shrayolacrayon
approved these changes
Mar 1, 2019
desimone
reviewed
Mar 2, 2019
| if o.upstreamConfigs != nil { | ||
| for _, uc := range o.upstreamConfigs { | ||
| if uc.Timeout > o.TCPWriteTimeout { | ||
| o.TCPWriteTimeout = uc.Timeout |
There was a problem hiding this comment.
What if TCPWriteTimeout is set to 0 to support streaming?
There was a problem hiding this comment.
I think users will have to implement this at their own peril for the moment. I think we'd have to implement an actually underlying idle tcp connection -- but might be a more involved change.
For the moment, this fix delivers tangible value to users.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
In #151, a problem was explored where SSO Proxy writes out
200 OKon long requests but nothing is written out to the client. We were able to trace this issue down to a default configuration we specify for thehttp.Serverthat would timeout tcp connections after the timeout window specified but before the upstream response was received.However, because of a bug in the golang stdlib golang/go#21389, when these writes occurred we were unable to easily diagnose the connection had closed and that the write was unsuccessful - no error is returned nor is there any useful log output.
Unfortunately, there is no way to cleanly handle this error case to make debugging more useful without re-writing portions of the
http.Server.Solution
For now, we can make this edge case a little less sharp by handling these timeouts more intuitively. We set the
WriteTimeoutfor thehttp.Serverto now be the max of the either the default value configured or the max upstream timeout config.Notes
Many thanks to @victornoel for raising both the issue and the mitigation implemented here.