From 1396eb7022faec4c2d91553800a35781a96dd5bd Mon Sep 17 00:00:00 2001
From: Tim Abbott <tabbott@mit.edu>
Date: Fri, 12 Feb 2016 21:34:14 -0800
Subject: [PATCH] Fix Tornado memory leak of handler objects.

In 2ea0daab193834bbffa56cc8e53e66e29c840e57, handlers were moved to
being tracked via the handlers_by_id dict, but nothing cleared this
dict, resulting in every handler object being leaked.  Since a Tornado
process uses a different handler object for every request, this
resulted in a significant memory leak.  We fix this by clearing the
handlers_by_id dict in the two code paths that would result in a
Tornado handler being de-allocated: the exception codepath and the
handler disconnect codepath.

Fixes #463.
---
 zerver/lib/event_queue.py                | 7 ++++---
 zerver/lib/handlers.py                   | 3 +++
 zerver/management/commands/runtornado.py | 5 ++++-
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/zerver/lib/event_queue.py b/zerver/lib/event_queue.py
index 280378629e..e31dcbe307 100644
--- a/zerver/lib/event_queue.py
+++ b/zerver/lib/event_queue.py
@@ -20,7 +20,8 @@ from zerver.decorator import RespondAsynchronously, JsonableError
 from zerver.lib.cache import cache_get_many, message_cache_key, \
     user_profile_by_id_cache_key, cache_save_user_profile
 from zerver.lib.cache_helpers import cache_with_key
-from zerver.lib.handlers import get_handler_by_id, finish_handler
+from zerver.lib.handlers import clear_handler_by_id, get_handler_by_id, \
+    finish_handler
 from zerver.lib.utils import statsd
 from zerver.middleware import async_request_restart
 from zerver.lib.narrow import build_narrow_filter
@@ -160,7 +161,7 @@ class ClientDescriptor(object):
 
     def disconnect_handler(self, client_closed=False):
         if self.current_handler_id:
-            delete_descriptor_by_handler_id(self.current_handler_id, None)
+            clear_descriptor_by_handler_id(self.current_handler_id, None)
             if client_closed:
                 logging.info("Client disconnected for queue %s (%s via %s)" %
                              (self.event_queue.id, self.user_profile_email,
@@ -184,7 +185,7 @@ def get_descriptor_by_handler_id(handler_id):
 def set_descriptor_by_handler_id(handler_id, client_descriptor):
     descriptors_by_handler_id[handler_id] = client_descriptor
 
-def delete_descriptor_by_handler_id(handler_id, client_descriptor):
+def clear_descriptor_by_handler_id(handler_id, client_descriptor):
     del descriptors_by_handler_id[handler_id]
 
 def compute_full_event_type(event):
diff --git a/zerver/lib/handlers.py b/zerver/lib/handlers.py
index 125d1aae7d..e30bc7a739 100644
--- a/zerver/lib/handlers.py
+++ b/zerver/lib/handlers.py
@@ -14,6 +14,9 @@ def allocate_handler_id(handler):
     current_handler_id += 1
     return handler.handler_id
 
+def clear_handler_by_id(handler_id):
+    del handlers[handler_id]
+
 def finish_handler(handler_id, event_queue_id, contents, apply_markdown):
     err_msg = "Got error finishing handler for queue %s" % (event_queue_id,)
     try:
diff --git a/zerver/management/commands/runtornado.py b/zerver/management/commands/runtornado.py
index 474de05d05..e01c1a8887 100644
--- a/zerver/management/commands/runtornado.py
+++ b/zerver/management/commands/runtornado.py
@@ -22,7 +22,7 @@ from zerver.lib.debug import interactive_debug_listen
 from zerver.lib.response import json_response
 from zerver.lib.event_queue import process_notification, missedmessage_hook
 from zerver.lib.event_queue import setup_event_queue, add_client_gc_hook, \
-    get_descriptor_by_handler_id
+    get_descriptor_by_handler_id, clear_handler_by_id
 from zerver.lib.handlers import allocate_handler_id
 from zerver.lib.queue import setup_tornado_rabbitmq
 from zerver.lib.socket import get_sockjs_router, respond_send_message
@@ -151,6 +151,8 @@ class AsyncDjangoHandler(tornado.web.RequestHandler, base.BaseHandler):
         self.initLock.release()
         self._auto_finish = False
         self.client_descriptor = None
+        # Handler IDs are allocated here, and the handler ID map must
+        # be cleared when the handler finishes its response
         allocate_handler_id(self)
 
     def get(self, *args, **kwargs):
@@ -243,6 +245,7 @@ class AsyncDjangoHandler(tornado.web.RequestHandler, base.BaseHandler):
                             async_request_stop(request)
                             return None
                     except Exception as e:
+                        clear_handler_by_id(self.current_handler_id)
                         # If the view raised an exception, run it through exception
                         # middleware, and if the exception middleware returns a
                         # response, use that. Otherwise, reraise the exception.