Reference cycles keep closed connections alive until a full GC pass

[peteralm80]

Closed connections (asyncio, sync, and trio implementations alike) are never
freed by reference counting: two reference cycles keep every connection alive
until the cyclic garbage collector runs. On servers with many connections and
high connect/disconnect churn, this causes very large memory growth, because
CPython 3.13+ collects the oldest generation incrementally and lags far behind
the churn on large heaps.

Observed in production (websockets 15.0.1, CPython 3.14.4, ~450 concurrent
connections with heavy churn): a gc census on the live process showed 13,589
ServerConnection and 3,335 SSLProtocol instances alive with only 453
connections actually open. A forced gc.collect() freed ~1.75 GB — and took
16 seconds, a stop-the-world pause long enough to break the process's other
websocket connections via ping timeout.

Both cycles affect every connection — graceful close and abrupt abort alike.
Verified on 15.0.1 and reproduced on current main.

Cycle 1: LoggerAdapter (all implementations)

Connection.__init__ injects the connection into the protocol's logger:

self.protocol.logger = logging.LoggerAdapter(
    self.protocol.logger,
    {"websocket": self},
)

connection → protocol → logger (LoggerAdapter) → extra dict → connection.
Nothing ever clears it. Disabling logging as documented (NullHandler /
propagate=False / setLevel) does not help — the cycle is created at
construction regardless of logging activity. The same pattern exists in
asyncio/connection.py, sync/connection.py, trio/connection.py (and
legacy/protocol.py, deprecated).

This was suspected once before in #1059, where @aaugustin noted: "I'd be open
to tweaking this with a weakref to facilitate garbage collection."

Cycle 2: cancelled keepalive task (asyncio implementation)

Connection.connection_lost() cancels self.keepalive_task but keeps the
attribute. A cancelled asyncio.Task retains its CancelledError, whose
__traceback__ holds the Connection.keepalive() coroutine frame, whose
locals include self:

connection → task → CancelledError → traceback → frame → connection.

Reproduction

Prints alive: True on current main; only a gc.collect() frees the
connection:

import asyncio, gc, weakref
from websockets.asyncio.client import connect
from websockets.asyncio.server import serve

async def handler(ws):
    try:
        async for _ in ws:
            pass
    except Exception:
        pass

async def main():
    gc.disable()
    gc.collect()
    server = await serve(handler, 'localhost', 8765)
    ws = await connect('ws://localhost:8765')
    await asyncio.sleep(0.2)
    ref = weakref.ref(next(iter(server.connections)))
    ws.transport.abort()          # also reproduces with: await ws.close()
    await asyncio.sleep(0.5)
    print('alive without gc:', ref() is not None)   # True == bug
    server.close()
    await server.wait_closed()

asyncio.run(main())

Break-set experiment on the dead connection (gc still disabled): clearing only
the logger cycle (conn.protocol.logger.extra.clear()) leaves the connection
alive; clearing the logger cycle and dropping the task
(conn.keepalive_task = None) makes the weakref die immediately. Both cycles
must be broken.

Related

recv_exc stores arbitrary exceptions whose tracebacks can also pin frames
referencing the connection (e.g. exceptions raised inside send_context()).
This wasn't required to fix the common paths above, but may be worth
considering separately (exc.__traceback__ = None would keep the cause chain
intact for close_exc.__cause__ while dropping the frames).

[aaugustin]

Hello, thank you for making a convincing case for this improvement, backed by production numbers.

I think it's worth addressing. I'll treat it as an improvement rather than a bug, though, because it's a performance optimization and you could also optimize by setting smaller gc thresholds.

Initial review notes for the asyncio implementation, likely applying to the other implementations too:

• What's your take on why self.keepalive_task = None is safe? I'm willing to believe that it is, but I'd like to be clear on why.
• I wish there was more concise solution than ConnectionLoggerAdapter, but I'm not immediately seeing one, and it's not too bad.
• Why did you decide against giving recv_exc the same treatment? It's quite likely to happen if you have high connection churn.
• The test is the right kind of test, but why is it in a new class instead of reusing the existing test infrastructure? Is there a real reason why you had to duplicate all the setup code? Or is this just AI slop?
• Your problem was specifically in the scenario of a server. Did you consider adding a test in the context of a server, to make sure that the connection isn't kept alive by something else? If yes, why did you decide against it?

[peteralm80]

The test is the right kind of test, but why is it in a new class instead of
reusing the existing test infrastructure?

Fixed. The tests moved into ClientConnectionTests and reuse its fixture;
teardown gets a guard because the tests must delete the fixture's reference to
the connection. ServerConnectionTests inherits them, so they also run
against a server-side connection now.

What's your take on why self.keepalive_task = None is safe? I'm willing to
believe that it is, but I'd like to be clear on why.

The cycle being broken is: connection → keepalive_task → _cancelled_exc
(the task stores the CancelledError so that awaiting it can re-raise) →
__traceback__ → keepalive() frame → connection. Dereferencing the task is
safe because:

1. The cancellation is still delivered. From cancel() until the task
   completes, the event loop holds a strong reference to it: cancelling a task
   that awaits a future schedules Task.__wakeup with call_soon(); a task
   that hasn't started yet is still scheduled by create_task(). Verified
   with gc disabled, for both a running and a never-started task: the
   finally clause runs, then the task is freed by reference counting.
2. Nothing reads keepalive_task after connection_lost(), which runs at
   most once. The attribute is written in __init__() and start_keepalive()
   and read only in connection_lost(). start_keepalive() can't run after
   connection_lost() on the server side due to the state is not OPEN check
   in protocol_handler(); on the client side, if the connection dropped
   between the handshake and start_keepalive(), connection_lost() already
   ran with keepalive_task still None — same behavior as before this
   change.
3. Nothing needs the task later: no code awaits it or retrieves its result,
   and a cancelled task doesn't log "Task exception was never retrieved".

The comment in connection_lost() now summarizes this.

Why did you decide against giving recv_exc the same treatment? It's quite
likely to happen if you have high connection churn.

Complexity — see below; I didn't want it to hold up the simpler fixes. It's
now the second commit of the branch; I can move it to a separate PR if you
prefer to land them separately.

recv_exc can't be dereferenced: it must outlive teardown so that later
recv()/send() calls raise ConnectionClosed chained to the original
error. The cycle is in its traceback: frames of connection methods hold
self in their locals. Which paths cycle:

• asyncio, peer resets the TCP connection (the common churn case): no cycle.
  ECONNRESET is raised and caught inside the transport; the traceback
  references the transport, which drops its protocol reference right after
  connection_lost(). Verified with an actual RST (SO_LINGER).
• asyncio, write error while answering an incoming frame: cycles via the
  data_received() and send_data() frames.
• sync and trio, any read error including ECONNRESET: cycles, because reads
  happen in recv_events(), a method of the connection.

The fix is traceback.clear_frames(): it drops frame locals, and formatting
is unchanged since it only needs code objects and line numbers. Frames can
only be cleared after they finish running, so placement differs:

• asyncio: in connection_lost() — all setters of recv_exc ran earlier.
• sync: after recv_events() returns, in RecvEventsThread.run(). The
  thread also dereferences the connection, like Thread.run() dereferences
  self._target, because the frame that called recv_events() is retained
  via the f_back attribute of the frame in the traceback.
• trio: same constraint, handled in a run_recv_events() wrapper; no
  dereference needed because coroutine frames don't retain f_back after
  finishing.

The trio regression test caught a third cycle, unrelated to recv_exc: the
assembler holds pause=self.pause_reading / resume=self.resume_reading, so
every trio connection was in a cycle regardless of how it closed (asyncio
passes transport methods, sync passes lock methods). Assembler.close() now
drops the callbacks.

Tests: test_garbage_collection_after_network_error for asyncio
(transport.write_eof(), same injection as
test_writing_in_data_received_fails), sync (sendall raising
BrokenPipeError), and trio (send_stream.close()); plus
test_garbage_collection_after_close for trio. All fail without the fix.

Protocol.parser_exc pins the sans-io protocol object (its traceback
references frames of parse()) but not the connection — except on Python
3.12, see below. I can give it the same treatment if you think it's worth it.

Your problem was specifically in the scenario of a server. Did you consider
adding a test in the context of a server, to make sure that the connection
isn't kept alive by something else?

Added test_garbage_collection_after_close to test_server.py for the three
implementations: serve() a server, connect a client, take a weak reference
to the ServerConnection from server.all_connections, disable gc, close
the client, wait for the connection handler (task or thread) to terminate,
assert the weak reference is dead. This covers the references kept by the
server — handler task/thread, all_connections,
handler_tasks/handler_threads — on top of the cycles internal to the
connection. All three fail without the fix; no flakiness in 30 repeat runs.

Python 3.12. The garbage collection tests are skipped on 3.12. On that
version — not 3.11 or 3.13+ — when the parser fails, parser_exc's traceback
keeps the frames of receive_data()/receive_eof()'s callers alive via
f_back, including connection methods, so closed connections stay in a cycle
no matter what the connection layer does. (Trigger: fail() creates the
discard() coroutine inside parse()'s except block; 3.13 fixed the frame
behavior. clear_frames() doesn't help: suspended generator frame, retention
via f_back.) Dropping parser_exc.__traceback__ on 3.12 would work but
costs exception reporting, so the tests are skipped instead:
skip_unless_reference_counting_collects in tests/utils.py. Also,
unittest.skipIf() on ≤ 3.12 replaces coroutine functions with regular
functions, which tripped IsolatedTrioTestCase; it now leaves skipped tests
unconverted.

[aaugustin]

Am I talking to a human, or just Claude Code over GitHub? Asking 'cause it's much faster to talk to Claude Code locally...

[peteralm80]

The issue was found manually in our production environment as well as the initial fix, the tests + the github issue was created by claude. I can certainly admit that I didn't look careful enough on how well the first iteration of the tests fit in with the rest of the test suite, but they did confirm the initial behavior and verified the fix.

Do what you will with the pull request - we're happy to keep the patch in house, but thought we'd share it upstream while wasting minimal time on writing issues and digging into test harnesses.