Skip to content

Break reference cycles keeping closed connections alive - #1750

Open
peteralm80 wants to merge 1 commit into
python-websockets:mainfrom
peteralm80:fix-connection-reference-cycles
Open

Break reference cycles keeping closed connections alive#1750
peteralm80 wants to merge 1 commit into
python-websockets:mainfrom
peteralm80:fix-connection-reference-cycles

Conversation

@peteralm80

Copy link
Copy Markdown

Fixes #1749.

Closed connections could only be freed by the cyclic garbage collector, never
by reference counting, because of two reference cycles (details, production
numbers, and reproduction in the issue):

  1. The LoggerAdapter that injects the websocket attribute into log records
    held a strong reference to the connection (all implementations).
  2. In the asyncio implementation, the cancelled keepalive task retained a
    CancelledError whose traceback references the keepalive() frame and
    thus the connection.

Changes:

  • New ConnectionLoggerAdapter in websockets/utils.py holds a weak
    reference to the connection and injects it into log records at logging time
    (the weakref approach suggested in Reference cycle in websockets 10? #1059). Behavior is unchanged for any
    record emitted while the connection object is alive — including "connection
    handler failed" in the server, since the handler still references the
    connection at that point — because each record receives its own strong
    reference via extra. The documented adapter-wrapping pattern
    (kwargs["extra"]["websocket"] with except KeyError) keeps working.
    Used by the asyncio, sync, and trio implementations. The deprecated legacy
    implementation is left untouched.

  • Connection.connection_lost() (asyncio) dereferences keepalive_task after
    cancelling it, so the task, its CancelledError, and the pinned frame die
    by refcount once the event loop finishes the cancellation.

Tests:

  • Regression tests in tests/asyncio/test_connection.py (graceful close and
    abort) and tests/sync/test_connection.py assert with gc.disable() in
    effect that a weakref to a closed connection dies — i.e. the connection is
    freed by reference counting alone. They fail without the fix and are skipped
    on non-CPython implementations.
  • Unit tests for ConnectionLoggerAdapter in tests/test_utils.py.
  • Two existing keepalive tests that read keepalive_task after close now
    capture the task before closing.

🤖 Generated with Claude Code

Closed connections could only be freed by the cyclic garbage collector,
never by reference counting, because two reference cycles referenced
the connection:

* The LoggerAdapter that injects the websocket attribute into log
  records held a strong reference to the connection, in all
  implementations: connection -> protocol -> logger -> extra dict ->
  connection.

* In the asyncio implementation, the keepalive task retained the
  CancelledError raised when connection_lost() cancelled it, whose
  traceback references the keepalive() frame and thus the connection:
  connection -> task -> exception -> traceback -> frame -> connection.

On servers handling many connections with high connect/disconnect
churn, closed connections accumulated until a full collection of the
oldest generation, which can lag far behind and pause the event loop
for several seconds on large heaps, especially since CPython 3.13
collects the oldest generation incrementally.

ConnectionLoggerAdapter now holds a weak reference to the connection
and injects it into log records only while the connection is alive;
records created while the connection is alive keep their own strong
reference, so logging filters and handlers are unaffected.
connection_lost() also dereferences the keepalive task after
cancelling it.
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.

Reference cycles keep closed connections alive until a full GC pass

2 participants