Skip to content

Bound Chat.leave_room() with a timeout, matching join_room() - #365

Open
janovak wants to merge 1 commit into
Teekeks:masterfrom
janovak:fix/leave-room-timeout
Open

Bound Chat.leave_room() with a timeout, matching join_room()#365
janovak wants to merge 1 commit into
Teekeks:masterfrom
janovak:fix/leave-room-timeout

Conversation

@janovak

@janovak janovak commented Aug 12, 2026

Copy link
Copy Markdown

Re-opening #364, which GitHub auto-closed when I deleted the fork it was based on. Same commit; the description below adds the precise mechanism, which I have since confirmed in the source.

Problem

Chat.leave_room() waits on _room_leave_locks with no deadline:

# wait to leave all rooms
while any([r in self._room_leave_locks for r in target]):
    await asyncio.sleep(0.01)

join_room() already protects the equivalent wait with join_timeout:

timeout = datetime.datetime.now() + datetime.timedelta(seconds=self.join_timeout)
while any([r in self._room_join_locks for r in target]) and timeout > datetime.datetime.now():
    await asyncio.sleep(0.01)

If the connection dies (or gets silently replaced by a reconnect) while a leave_room() call is mid-wait, the PART confirmation that would clear its lock never arrives — and the call hangs forever, along with anything awaiting it.

Why a reconnect is enough to trigger it

The lock is only ever cleared in two places: on the PART confirmation in _handle_part, or by a wholesale reset of the list. That reset lives in _stop() — the full-shutdown path:

async def _stop(self):
    ...
    self._room_join_locks = []
    self._room_leave_locks = []

__connect() — the reconnect path, reached via _handle_base_reconnect() — does not reset those lists. So a reconnect discards the in-flight PART confirmation while leaving the lock behind, and the while loop above can never exit.

This makes no_message_reset_time a direct amplifier. It becomes the websocket receive_timeout:

receive_timeout = None if self.no_message_reset_time is None else self.no_message_reset_time * 60

so any quiet period longer than that forces a reconnect. Lowering it to catch dead sockets faster (we run 30s) raises the chance of a reconnect landing inside a leave_room() call. The failure is also self-reinforcing: fewer messages → more reconnects → higher chance of the hang → our watchlist freezes → fewer messages still.

How we hit this

We run Chat in a service that periodically joins/leaves rooms based on a rotating watchlist. A transient asyncio SSL-transport error (a known, sporadic issue — see cpython#81407 / cpython#85762) killed the underlying connection while a leave_room() call was in flight. Because leave_room() has no timeout, that single call blocked forever, which wedged the scheduled task that owned it — with no exception raised anywhere for calling code to catch. We only recovered by restarting the process.

It happened twice in one day, hanging for 1h15m and 3h10m before we caught it. Because our other rooms stayed joined and kept delivering messages, there was no crash and no error log — throughput just decayed quietly over several hours as streams ended and the frozen watchlist could not refresh.

join_room() would have survived the identical failure by simply giving up after join_timeout (10s) and returning the channels it couldn't join.

Fix

Give leave_room() the same protection join_room() already has:

  • Add leave_timeout (default 10, matching join_timeout's default)
  • Bound the wait loop with it
  • Clear any locks left over from channels that timed out (mirrors join_room()'s cleanup of _room_join_locks)
  • Return the list of channels that couldn't be left in time, matching join_room()'s failed_to_join return contract

Additive/non-breaking on the success path — with the default leave_timeout=10, behavior is identical to today as long as the PART confirmation arrives within 10s, which is the case in the overwhelming majority of calls.

Worth noting this cannot be fixed correctly from the caller's side: wrapping leave_room() in asyncio.wait_for cancels it from outside, which leaves the channel's entry in _room_leave_locks permanently — the same corruption, just relocated. The cleanup has to happen inside the method.

Testing

Running this patch in production against the stock 4.5.0 release, in a service that continuously joins/leaves 15-30 rooms. It has been deployed for a short time so far (rolled out today), so I would not yet call the soak long — but normal join/leave behavior is unaffected, and the hang has not recurred since. Before the patch, on the identical workload and stock 4.5.0, we reproduced the hang twice in a single day.

leave_room() waits on _room_leave_locks with no deadline, unlike
join_room() which already bounds its equivalent wait with
join_timeout. If the underlying connection dies (or is silently
replaced by a reconnect) while a leave_room() call is mid-wait, the
PART confirmation that would clear its lock never arrives, and the
call -- and anything awaiting it -- hangs forever.

Add a leave_timeout attribute (default 10s, same default as
join_timeout) and bound the wait loop with it, returning the list of
channels that couldn't be left in time, mirroring join_room()'s
existing contract.
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