Drop ZMQ sends after stop, so a late announce cannot leak a socket - #36
Merged
Merged
Conversation
prkrtg
approved these changes
Sep 17, 2026
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
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.
Surfaced by the keygrabber's tests, which create and close a ZMQ
Clientper case and produced aResourceWarningfor an unclosed DEALER.ZmqTransport.sendreturns early once_stopis set. Sending creates a DEALER per peer on demand andstop()closes the ones that exist, so a later send opened one that nothing would ever closeDiscovery.stop()only sets a flag and never joins its thread, and that thread sits intime.sleep(every_seconds). So up to one announce interval afterLibby.stop()closed every socket, discovery can still broadcast.Client.zmqenables discovery by default with a 2s interval, so every stopped ZMQ client could leak one DEALER per address-book entrytests/test_zmq_transport.py. One case asserts a live broadcast does open a dealer, so the leak is demonstrably reachable rather than hypothetical, and the rest assert nothing is opened after stop. Confirmed they fail without the guard-W error::ResourceWarning, which it did not beforeWorth knowing separately: ZMQ's
broadcast:*only iterates the sender's own address book, sodiscover=Trueover ZMQ announces this peer to peers it was already told about and never learns a new one. Over RabbitMQ the same call fans out through an exchange. The name suggests more symmetry than there is.Also noticed while opening this: #35 widened
pull_requestto every base branch, but thepathslists still omit.github/workflows/**, so a workflow-only PR triggers no checks. That is why #35 itself reported none. Not fixed here.