Skip to content

connmgr: Misc test cleanup and modernization.#3699

Open
davecgh wants to merge 23 commits into
decred:masterfrom
davecgh:connmgr_test_cleanup
Open

connmgr: Misc test cleanup and modernization.#3699
davecgh wants to merge 23 commits into
decred:masterfrom
davecgh:connmgr_test_cleanup

Conversation

@davecgh

@davecgh davecgh commented May 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

This requires #3698.

This consists of a few commits to modernize and do some light cleanup the connection manager tests.

  • Separate code related to mock addresses, connections, and listeners to its own file
  • Use t.Cleanup to wait for clean shutdown and assert final state correctness
  • Make variable names in tests consistent with the implementation code
  • Only close once in double close tests
  • Use error return from Connect for dial timeout detection

@davecgh davecgh added this to the 2.2.0 milestone May 23, 2026
@davecgh davecgh force-pushed the connmgr_test_cleanup branch from 96f057d to 21267c7 Compare May 24, 2026 22:48
@davecgh davecgh mentioned this pull request May 24, 2026
Open
@davecgh davecgh force-pushed the connmgr_test_cleanup branch from 21267c7 to acd90f5 Compare May 26, 2026 00:33
@davecgh davecgh force-pushed the connmgr_test_cleanup branch from acd90f5 to 1dafa76 Compare June 4, 2026 04:00
jholdstock
jholdstock reviewed Jun 8, 2026
View reviewed changes
Comment thread internal/connmgr/connmanager_test.go Outdated
@davecgh davecgh force-pushed the connmgr_test_cleanup branch 2 times, most recently from b811823 to 1ce361d Compare June 8, 2026 03:57
@davecgh davecgh force-pushed the connmgr_test_cleanup branch from 1ce361d to 8bd2954 Compare July 1, 2026 02:34
matthawkins90
matthawkins90 reviewed Jul 4, 2026
View reviewed changes
Comment thread internal/connmgr/connmanager_test.go Outdated
@davecgh davecgh force-pushed the connmgr_test_cleanup branch from 8bd2954 to e64efdc Compare July 4, 2026 19:27
davecgh added 17 commits July 5, 2026 21:08
@davecgh
connmgr: Support whitelisting.
c473cdf 
Currently the whitelisting logic happens in the server which makes it
inaccessible to the connection manager.

In order to pave the way for supporting various connection-related logic
that currently happens in the server, but ideally should be happening in
the connection manager, this adds basic support for whitelisting CIDR
prefixes to the connection manager.

The connection manager config struct now accepts a slice of prefixes and
a new method named IsWhitelisted is added.

Note that this only adds support .  It does not update anything to use
the new functionality yet.
@davecgh
connmgr: Add whitelist detection tests.
220dea0 
This adds tests to ensure the new whitelist detection method works as
expected.
@davecgh
server: Integrate connmgr whitelisting.
45898a9 
This modifies the server to pass in the parsed whitelist entries to
the connection manager config and the relevant code to make use of the
new method it exposes.

Finally, it removes the no longer used local isWhitelisted method.
@davecgh
connmgr: Update README.md for whitelist support.
600d4df
@davecgh
connmgr: Add try acquire support to semaphore.
6003b59 
This adds a new TryAcquire method to the context-aware semaphore.  As
the name implies, the method supports conditionally acquiring the
semaphore only when resources are immediately available.  In other
words, it will not block when there are no resources immediately
available.
@davecgh
connmgr: Add semaphore try acquire tests.
7730601 
This adds tests for the new TryAcquire method on the context-aware
semaphore to ensure the semantics work as expected.
@davecgh
connmgr: Limit total overall normal connections.
627bf80 
The current overall total connection limits are enforced by the server
rather than the connection manager.  This is not ideal for many reasons,
but one of the most important consequences is that it makes DoS attacks
easier.  Another example of some less than ideal behavior that it allows
is that some rare combinations of events can lead to temporary extra
connection churn.

It is much more robust and natural to perform the limiting in the
connection manager itself via semaphores.  That approach not only
significantly hardens the server against DoS attacks and solves various
edge cases present in the current code, it also paves the way for even
more advanced features such as traffic shaping in the future.

To that end, this adds semaphore-based limiting for the total overall
number of normal connections to the connection manager and removes the
relevant current limiting for it from the server.

Normal connections are the automatic outbound, manual outbound, and
inbound connections.  Persistent connections, on the other hand, are not
subject to the limit since they have their own limiting.  This is
consistent with them not being subject to the automatic target outbound
limit either.
@davecgh
connmgr: Add total max normal conns tests.
a347447 
This adds tests to ensure that the new max normal connection limiting
properly enforces the limit including automatic outbound, manual
outbound, and inbound connections.

It also ensures that it not applied to persistent connections.
@davecgh
connmgr: Update README.md for total conn limits.
b525f3c
@davecgh
connmgr: Wait for all goroutines to finish.
ed8d59b 
This modifies the persistent connection and target outbound handler
shutdown logic to wait for any goroutines they have launched to finish
before returning.

This ensures there are no dangling goroutines from them once Run
returns.
@davecgh
connmgr: Use concrete addrmgr types in test.
88406e8 
This updates the primary parsing method in the connection manager tests
to return a concrete addrmgr address instead of a stdlib net.TCPAddr.

The goal is to eventually use the concrete address type almost
everywhere to avoid a lot of the less than ideal address reparsing and
repeated host/port splitting and joining.
@davecgh
connmgr: Use concrete addr type more often.
0635bd3 
This modifies the address handling to parse the stdlib net.Addr to a
concrete addrmgr address earlier in the connect and persistent paths
rather than doing it in the dial method and switch the callback to get
new addresses to return the concrete type.

The server is updated to simply the return the chosen address which no
longer needs to be converted.

Note that this is theoretically a semantics change because the code in
server previously potentially resolved the address via DNS and that no
longer is the case.

In practice, nothing is really changing in terms of resolution though
because the address manager only ever works with resolved addresses
since it needs to gossip addresses via the wire protocol which only
deals with resolved addresses.
@davecgh
connmgr: Limit max connections per host.
1c0c23a 
Similar to the recent total normal connection limiting, the current
per-host connection limits are enforced by the server.  For similar
reasons, it is much more robust and natural to perform the limiting
early in the connection manager.

With that in mind, this implements the per-host connection limiting in
the connection manager and removes the relevant current limiting for it
from the server.  The limiting is applied to inbound, outbound, and
persistent connections.

The new limiting is handled early in both the inbound and outbound paths
now which allows it to take advantage of fast connection shedding for
inbound connections and to preemptively prevent all outbound attempts
that would exceed the limit regardless of source.  It also provides the
flexibility to apply independent special permissions in the future.

This also slightly changes the semantics to exempt whitelisted addresses
for both inbound and outbound connections as opposed to only inbound
connections.
@davecgh
connmgr: Add max per-host conn tests.
0015801 
This adds tests to ensure that the new max connections per host limiting
properly enforces the limit including automatic outbound, manual
outbound, inbound, and persistent connections.  It also tests
whitelisted addresses are exempt.
@davecgh
connmgr: Update README.md for per-host conn limits.
fdfca08
@davecgh
connmgr: Rework to use per-host permits -- to be squashed
9c4999e
@davecgh
connmgr: Update tests for reworked per-host limit -- to be squashed
f997588
davecgh added 6 commits July 5, 2026 21:09
@davecgh
connmgr: Separate mock conn and addr test code.
ca629fc 
This moves the code related to mock addresses, connections, and
listeners to a separate file.  This helps keep it from cluttering up the
main test code and also makes it easier to reuse in other packages.
@davecgh
connmgr: Use more modern t.Cleanup in tests.
fb36add 
Most of the tests in this package were written well before some of the
more modern test conveniences like t.Cleanup were added.

As a result, almost all of the tests repeat the code related to waiting
for and asserting clean shutdown.

This updates the tests so that the main method used to run the
connection manager in all of the tests now registers a cleanup func via
t.Cleanup to wait for clean shutdown and assert the internal state is
clean as expected.

This approach is much less error prone and convenient.
@davecgh
connmgr: Consistent naming in tests.
2f41cec 
This makes the name of the connection manager instances in the test code
match the naming used in the implmentation code.
@davecgh
connmgr: Use more modern t.Context in tests.
e8c8987 
Most of the tests in this package were written when the connection
manager was based the older async model and before t.Context was
available.

As a result, the main method used to run the connection manager in all
of the tests takes a context that is always set to a new background
context.

This updates the method to remove the parameter and instead use the test
context via t.Context.
@davecgh
connmgr: Only close once in double close tests.
6a3fd54
@davecgh
connmgr: Cleaner dial timeout detection test.
dd4c264 
Now that the Connect method is synchronous and returns an error, this
modifies the test for detecting dial timeouts to use that error for more
more accurate detection that the failure is actually the result of dial
timeout as expected.
@davecgh davecgh force-pushed the connmgr_test_cleanup branch from e64efdc to dd4c264 Compare July 6, 2026 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@jholdstock jholdstock jholdstock approved these changes
@matthawkins90 matthawkins90 matthawkins90 approved these changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.2.0

Development

Successfully merging this pull request may close these issues.

3 participants

@davecgh @jholdstock @matthawkins90