Open
Conversation
This is the first in a series of commits that will ultimately convert all of the code related to connection manager handling events to make use of separate mutexes and atomics to protect concurrent access. The motivation for the change in architecture follows. Currently, nearly all operations performed by the connection manager are implemented via a single event handler goroutine and a single message channel to protect concurrent access. Generally speaking, mutexes and atomics are best suited to caches and state tracking, while channels are better suited to distributing units of work and communicating async results. While the existing implementation has worked well for many years, it has several downsides such as: - Adding any new functionality requires additional plumbing to provide access to any related information - The state is mostly inaccessible since it is limited to a single goroutine - The use of channels significantly hinders dynamically adjusting to changing conditions based on the state due to the previous There are several other improvements that would be ideal to make, but in the effort of making it easier to follow the changes and assert correctness, this series of commits focuses only on converting it to a synchronous model. With all of the in mind, the commit starts the conversion by introducing a separate mutex to protect the connection requests and moving the map that tracks the connection requests out of the event handler to the connection manager itself. This will allow future commits to methodically refactor the various operations without introducing races.
This moves the pending connection requests map out of the event handler goroutine to the connection manager itself and makes it concurrent safe by protecting it with the new connection mutex. This is part of the overall effort to convert the code related to handling the various connection manager events to synchronous code.
This refactors the logic related to registering a pending connection out of the event handler since it is now independently concurrent safe. This is a part of the overall effort to convert the code related to handling the various connection manager events to synchronous code.
This refactors the logic related to canceling a pending connection out of the event handler since it is now independently concurrent safe. This is a part of the overall effort to convert the code related to handling the various connection manager events to synchronous code.
This refactors the logic related to iterating active connections out of the event handler since it is now independently concurrent safe. This is a part of the overall effort to convert the code related to handling the various connection manager events to synchronous code.
This refactors the logic related to handled established connections out of the event handler since it is now independently concurrent safe. This is a part of the overall effort to convert the code related to handling the various connection manager events to synchronous code.
This refactors the logic related to handled failed connections out of the event handler since it is now independently concurrent safe. This is a part of the overall effort to convert the code related to handling the various connection manager events to synchronous code.
This refactors the logic related to disconnection out of the event handler since it is now independently concurrent safe. This is a part of the overall effort to convert the code related to handling the various connection manager events to synchronous code.
This removes the requests message channel and connection handler since they are no longer used. This is a part of the overall effort to convert the code related to handling the various connection manager events to synchronous code.
This makes TestPassAddrAlongDialAddr more accurate by explicitly detecting the dailed address directly in the provided dialer as opposed to from the conn request object of the mock connection. An invalid address, such as the one that is used in the test, would never result in a valid connection.
This reworks the TestTargetOutbound test a bit to ensure it fails if the expected number of connections are not made within a certain timeout. The prevents a test timeout in the failure case.
This removes all code related to DialAddr from the connection manager. Now the the connection manager is internal and the main code does not make use of DialAddr, there is no reason to keep a bunch of unused and unneeded code around.
davecgh
force-pushed
the
connmgr_remove_unused_dialaddr
branch
from
3393ac0 to
dd29140
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This requires #3646.
This removes all code related to
DialAddrfrom the connection manager.Now the the connection manager is internal and the main code does not make use of
DialAddr, there is no reason to keep a bunch of unused and unneeded code around.