From 579bdc18f85ea8599c8cf1f53ddb02fd41d97993 Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Thu, 7 Dec 2023 19:20:54 +0000 Subject: [PATCH] tornado: Consistently clear handler, via on_finish. initialize() is called on every request, and stored the `RequestHandler` (and thus `HTTPServerRequest`) in a global shared dict. However, the object is only removed from that structure if the request was successful. This means that failed requests (such as 405 Method Not Allowed) leaked `RequestHandler`s and `HTTPServerRequest`s. Move the cleanup to `on_finish`, which is called at the close of all requests, async and not, successful or not. --- zerver/tornado/event_queue.py | 8 +------- zerver/tornado/handlers.py | 8 ++++---- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/zerver/tornado/event_queue.py b/zerver/tornado/event_queue.py index 6f2f9c7f78..a9afd1108c 100644 --- a/zerver/tornado/event_queue.py +++ b/zerver/tornado/event_queue.py @@ -48,12 +48,7 @@ from zerver.middleware import async_request_timer_restart from zerver.models import CustomProfileField from zerver.tornado.descriptors import clear_descriptor_by_handler_id, set_descriptor_by_handler_id from zerver.tornado.exceptions import BadEventQueueIdError -from zerver.tornado.handlers import ( - clear_handler_by_id, - finish_handler, - get_handler_by_id, - handler_stats_string, -) +from zerver.tornado.handlers import finish_handler, get_handler_by_id, handler_stats_string # The idle timeout used to be a week, but we found that in that # situation, queues from dead browser sessions would grow quite large @@ -283,7 +278,6 @@ class ClientDescriptor: def disconnect_handler(self, client_closed: bool = False) -> None: if self.current_handler_id: clear_descriptor_by_handler_id(self.current_handler_id) - clear_handler_by_id(self.current_handler_id) if client_closed: logging.info( "Client disconnected for queue %s (%s via %s)", diff --git a/zerver/tornado/handlers.py b/zerver/tornado/handlers.py index 983e4deb7e..063db261aa 100644 --- a/zerver/tornado/handlers.py +++ b/zerver/tornado/handlers.py @@ -98,6 +98,10 @@ class AsyncDjangoHandler(tornado.web.RequestHandler): self._request: Optional[HttpRequest] = None + @override + def on_finish(self) -> None: + clear_handler_by_id(self.handler_id) + @override def __repr__(self) -> str: descriptor = get_descriptor_by_handler_id(self.handler_id) @@ -184,10 +188,6 @@ class AsyncDjangoHandler(tornado.web.RequestHandler): # For normal/synchronous requests that don't end up # long-polling, we just need to write the HTTP # response that Django prepared for us via Tornado. - - # Mark this handler ID as finished for Zulip's own tracking. - clear_handler_by_id(self.handler_id) - assert isinstance(response, HttpResponse) await self.write_django_response_as_tornado_response(response) finally: