transport: connection opening bugfixes #295
Open
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
WrapConn didn't close connReader and connWriter loops, leaving this responsibility to its callers, potentially leading to goroutine leaks, this is mitigated by closing those loops and never returning a non-nil Conn pointer when WrapConn fails. Fixes #289
Kulezi
force-pushed
the
pp/conn-opening-fixes
branch
from
76b095d to
cb19b3c
Compare
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.
The goal of this PR is to resolve 3 small bugs, all related to opening connections, especially sharded ones.
transport: WrapConn goroutine leak #289
This possible leak of goroutines was caused by OpenConn not closing the goroutines started by it immediately, returning a working connection with an error instead of closing it, leaving it to the caller. Now it's handled properly by closing it there.
I added a test checking if the behavior is correct when the handshake fails using a TCP server returning responses that would trigger that fail.
conn: OpenShardConn makes unnecessary retries #281
Now retries in OpenShardConn happen only when the error is either EADDRINUSE or EPERM as mentioned in transport: conn, abort OpenShardConn faster #286 (review)
transport: pool, stop shard refill faster when context is done #291
Now context.Err() is checked before each OpenShardConn call, ensuring we don't try opening connections after context is done,
The last 2 commits together reduce the integration test logs from ~40k lines to ~800 lines, this is a proof that context done actually cancels operations right away.
Fixes: #289
Fixes: #281
Fixes: #291