Skip to content

Release the socket, its loops and the session when disconnecting - #142

Open
deoxis9001 wants to merge 2 commits into
ArchipelagoMW:mainfrom
deoxis9001:fix/socket-teardown-leak
Open

deoxis9001 wants to merge 2 commits into
ArchipelagoMW:mainfrom
deoxis9001:fix/socket-teardown-leak

Conversation

@deoxis9001

Copy link
Copy Markdown

What happens today

An ArchipelagoSession is never released after DisconnectAsync, and one of the
three 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 WeakReference to each
session, 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 BaseArchipelagoSocketHelper

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, 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. 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.

3. The socket is never disposed

WebSocket is IDisposable, and Dispose appeared nowhere in the library.

The change

A CancellationTokenSource per helper, cancelled by DisconnectAsync and passed
to both loops; the send queue is completed; the socket is disposed.

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.

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:

before after
sessions still alive after gen2 91 / 91 0 (only the one being collected)
managed heap after a 91-slot read 2088 MB, rising every time 96-124 MB, falling
two 91-slot parties 4776 MB ~150 MB
time for a 91-slot read 192 s 124 s

What I could not verify

I did not run the test suite: it references the library through HintPath to the
net35/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.

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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant