fix leaking connections when user client closes connection #222
Conversation
This file contains hidden or 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
Signed-off-by: Teo Koon Peng <[email protected]>
…ctions Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
ensonic
reviewed
Sep 13, 2023
| // A permanent error suggests the request should be aborted. | ||
| log.Printf("[%s] %s", *resp.Id, err) | ||
| log.Printf("[%s] Closing backend connection", *resp.Id) |
There was a problem hiding this comment.
Please only log a single line:
log.Printf("[%s] Relay error: %v, closing backend connection", *resp.Id, err)
There was a problem hiding this comment.
updated in 301b04d
|
Thanks for moving the PR, the CI looks happy now. Will you also address the other comments from #220? |
Signed-off-by: Teo Koon Peng <[email protected]>
ensonic
reviewed
Sep 15, 2023
drigz
approved these changes
Sep 15, 2023
Merged
drigz
added a commit
that referenced
this pull request
Sep 15, 2023
It causes the postsubmit integration test to fail with: ``` error: Timeout occurred TEST FAILED: echo command did not run, output was "", want "hello" ``` when testing `kubectl exec`. https://source.cloud.google.com/results/invocations/8f0b86f7-c5c4-41a3-9e4c-43e325d9e1ae/targets/%2F%2Fsrc%2Fgo%2Ftests:relay_test/log - Revert "Use %v instead of %s in relay logs (#224)" - Revert "fix leaking connections when user client closes connection (#222)"
koonpeng
added a commit
that referenced
this pull request
Sep 18, 2023
drigz
pushed a commit
that referenced
this pull request
Sep 18, 2023
Re-reverting #222 with a fix to relay_test.sh from the postsubmit test. The problem is mistakenly using `w.Write` at server.go:260, the response writer is no longer valid after the connection has been hijacked, we need to use `bufrw.Write` instead. --------- Signed-off-by: Teo Koon Peng <[email protected]>
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.
Same as #220, but ported to this repo.
When the user client closes the connection, it is not propagated to the backend. For simple "one shot" requests, there is no huge impact as the response would just be dropped, however for long running requests (chunked encoding or websockets), the backend doesn't know that the user client has closed the connection and it keeps sending new updates to the relay client, which also doesn't know and so keep forwarding that to the relay server.
The fix applied is:
StopRelayRequestto the broker to "forget" a relaying request. This will cause the relay server to response with a permanent error the next time the relay client tries to send a response for that id. This will in turn cause the relay client to close the connection to the backend.backoff.Permanent, resulting in the check to always fail and never handling it.