From c741c527d7cc56d7a4999257b7332ce2441bf63b Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Tue, 12 Dec 2023 04:32:05 +0000 Subject: [PATCH] tornado: Support clearing a handler more than once. 4af00f61a8c4b779365a5e7b8746716b7f32e837 claimed that `on_finish` and `on_connection_close` were mutually exclusive. In cases where a `DELETE` is called on the queue while a longpoll is in progress, this can cause _both_ to happen: - The `DELETE` pushes a `cleanup_queue` event, which triggers `finish_handler` to begin pushing out an empty event response to the longpoll connection. - In the midst of that, in an `await`, the longpoll connection drops, and `on_connection_close` clears the handler. - The `await` resumes, calls `finish`, and attempts to clear the handler. The easiest solution is to make `clear_handler_by_id` tolerant to multiple attempts to clear it. Since these processes run in parallel, it means that parts may have a `handler_id` but `get_handler_by_id` may error in attempting to look it up. We have not observed this in testing, and I cannot currently prove it is impossible. --- zerver/tornado/handlers.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/zerver/tornado/handlers.py b/zerver/tornado/handlers.py index 1f1fc13d36..00a1f9694e 100644 --- a/zerver/tornado/handlers.py +++ b/zerver/tornado/handlers.py @@ -37,7 +37,8 @@ def allocate_handler_id(handler: "AsyncDjangoHandler") -> int: def clear_handler_by_id(handler_id: int) -> None: - del handlers[handler_id] + if handler_id in handlers: + del handlers[handler_id] def handler_stats_string() -> str: