Repository navigation
Release the socket, its loops and the session when disconnecting - #142
Open
deoxis9001 wants to merge 2 commits into
Open
deoxis9001 wants to merge 2 commits into
deoxis9001 wants to merge 2 commits into
Conversation
An ArchipelagoSession is never released after DisconnectAsync. Measured on a Discord bot that opens one session per slot to read each player's received items at startup: a 91-slot multiworld leaves ~1.8 GB behind, and it accumulates with every reconnection. A weak reference to each session reports 91 out of 91 still alive after a blocking, compacting, aggressive gen2 collection. Three causes, all in this file. 1. SendLoop blocks a thread pool thread forever. HandleSendBuffer starts with `sendQueue.Take()`. Take blocks the calling thread while the collection is empty, and CompleteAdding is never called anywhere in the library, so it never returns. Every socket that is opened costs one thread pool thread for the lifetime of the process. With enough sockets the pool starves, and since it only injects about one thread per second, unrelated work starts timing out -- websocket handshakes included. Take now takes the shutdown token, and DisconnectAsync completes the queue. 2. PollingLoop cannot be cancelled. ReadMessageAsync awaited `Socket.ReceiveAsync(..., CancellationToken.None)`. Closing the socket from another thread does not necessarily complete that read, and an async state machine that is awaiting is rooted by the runtime. Through the socket helper it holds the event handlers, therefore the session's helpers, therefore the session and its copy of the data package. The token is now passed down, and cancellation ends the loop quietly rather than being reported as an error -- it is the expected way out. 3. The socket is never disposed. WebSocket is IDisposable and Dispose appeared nowhere. DisconnectAsync now disposes it, after cancelling: disposing first would make the pending read fail with a disposal error instead of ending quietly. DisconnectAsync is also made idempotent, and its CloseAsync is bounded by a five second timeout -- it waits for the server's close frame, and a server that never answers would otherwise hang the call forever. No public API changes.
Measured on a full recompile: 536 errors -- 530 CS1591, 4 CS1570, 2 CS1574 -- all raised by this project's net6.0 target, in a solution whose own code is clean and which merely references it. -warnaserror is a global property. Passed on the consuming project's command line, it travels down the ProjectReference into this one, where GenerateDocumentationFile is on and 530 public members carry no XML comment. The consumer had written nothing wrong; its build was red all the same. It only surfaced on a FULL rebuild, which is why it had gone unnoticed. An incremental build does not recompile this project, so no diagnostic is re-emitted and the very same command returns 0. The consumer's own CI had never run from a cold checkout, and its local script -- the one that passes --no-incremental precisely to defeat that blind spot -- was the first thing to go red. NoWarn rather than TreatWarningsAsErrors=false: the latter is a project property that the global one overrides, whereas NoWarn suppresses the warnings before they can be promoted at all. The trade is stated, not hidden: these warnings are now silent when building this repository on its own too. The 530 undocumented public members remain a backlog. This commit does not reduce it -- it stops it from breaking other people's builds. No code and no public API changes. One rider, declared rather than left to be discovered: this file mixed CRLF and LF endings, and the two CRLF lines of the Icon folder ItemGroup came out as LF. Nothing but whitespace, and it makes the file consistent, but it is not part of the fix and it is not something the commit title would lead anyone to expect.
This branch has not been deployed
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.
What happens today
An
ArchipelagoSessionis never released afterDisconnectAsync, and one of thethree causes below costs a thread pool thread per socket for the lifetime of the
process.
Found on a Discord bot that opens one session per slot at startup, to read what
each player has received. On a 91-slot multiworld this left about 1.8 GB
behind, and it accumulated with every reconnection. A
WeakReferenceto eachsession, counted after a blocking, compacting, aggressive gen2 collection,
reported 91 out of 91 still alive — with 0 sockets still open. They were
properly disconnected, and rooted anyway.
Three causes, all in
BaseArchipelagoSocketHelper1.
SendLoopblocks a thread pool thread foreverHandleSendBufferstarts withsendQueue.Take().Takeblocks the callingthread while the collection is empty, and
CompleteAddingis never calledanywhere in the library, so it never returns.
Every socket that is opened costs one thread pool thread, permanently. With
enough sockets the pool starves, and since it only injects about one thread per
second, unrelated work starts timing out — websocket handshakes included. In our
case this, not the memory, was what made connections fail: after the fix the same
91-slot read went from 192 s to 124 s, a 35% gain with no other change.
2.
PollingLoopcannot be cancelledReadMessageAsyncawaitedSocket.ReceiveAsync(..., CancellationToken.None).Closing the socket from another thread does not necessarily complete that read,
and an async state machine that is awaiting is rooted by the runtime. Through the
socket helper it holds the event handlers, therefore the session's helpers,
therefore the session and its copy of the data package.
3. The socket is never disposed
WebSocketisIDisposable, andDisposeappeared nowhere in the library.The change
A
CancellationTokenSourceper helper, cancelled byDisconnectAsyncand passedto both loops; the send queue is completed; the socket is disposed.
DisconnectAsyncis also made idempotent, and itsCloseAsyncis bounded by afive second timeout — it waits for the server's close frame, and a server that
never answers would otherwise hang the call forever.
Order matters and is commented: cancel first, then dispose. Disposing first would
make the pending read fail with a disposal error rather than end quietly.
No public API changes.
Measured before and after
Same bot, same multiworld, same machine:
What I could not verify
I did not run the test suite: it references the library through
HintPathto thenet35/net40/net45 outputs, and those targeting packs are not available on my
machine. The change is confined to one file, and the numbers above come from
production rather than from unit tests.