Repository: aws/chalice

Issue:
app.current_request is not thread safe
I'm running into a problem when testing using chalice local and making calls to my service at almost the same time. It seems like app.current_request could get overwritten by a later request by the time I finish handling an earlier one so my current handler is working with a request object with context for the later request (i.e. I'm handling Request 1, Request 2 comes in, Request 3 comes in, my handler for request 2 uses app.current_request and gets the context for request 3). 

Would it be better to pass the request object in to the route?

Is this a potential problem when running in lambda or does each handler get a separate process?

Discussion:

Patch:
diff --git a/.changes/next-release/36408038949-bugfix-local-5121.json b/.changes/next-release/36408038949-bugfix-local-5121.json
new file mode 100644
index 0000000..7861edf
--- /dev/null
+++ b/.changes/next-release/36408038949-bugfix-local-5121.json
@@ -0,0 +1,5 @@
+{
+  "type": "bugfix",
+  "category": "local",
+  "description": "Make ``current_request`` thread safe in local mode (#759)"
+}
diff --git a/chalice/local.py b/chalice/local.py
index 3be170f..7e37d89 100644
--- a/chalice/local.py
+++ b/chalice/local.py
@@ -25,7 +25,6 @@ from typing import (
     Callable,
     Optional,
     Union,
-    cast,
 )  # noqa
 
 from chalice.app import Chalice  # noqa
@@ -58,9 +57,8 @@ class Clock(object):
 
 def create_local_server(app_obj, config, host, port):
     # type: (Chalice, Config, str, int) -> LocalDevServer
-    local_app_obj = LocalChalice(app_obj)
-    casted_local_app_obj = cast(Chalice, local_app_obj)
-    return LocalDevServer(casted_local_app_obj, config, host, port)
+    app_obj.__class__ = LocalChalice
+    return LocalDevServer(app_obj, config, host, port)
 
 
 class LocalARNBuilder(object):
@@ -708,30 +706,21 @@ class HTTPServerThread(threading.Thread):
             self._server.shutdown()
 
 
-class LocalChalice(object):
-    def __init__(self, chalice):
-        # type: (Chalice) -> None
-        self._current_request_lookup = {}  # type: Dict[int, Optional[Request]]
-        self._chalice = chalice
+class LocalChalice(Chalice):
+
+    _THREAD_LOCAL = threading.local()
+
+    # This is a known mypy bug where you can't override instance
+    # variables with properties.  So this should be type safe, which
+    # is why we're adding the type: ignore comments here.
+    # See: https://github.com/python/mypy/issues/4125
 
-    @property
-    def current_request(self):  # noqa
-        # type: () -> Optional[Request]
-        thread_id = threading.current_thread().ident
-        assert thread_id is not None
-        return self._current_request_lookup.get(thread_id, None)
+    @property  # type: ignore
+    def current_request(self):  # type: ignore
+        # type: () -> Request
+        return self._THREAD_LOCAL.current_request
 
     @current_request.setter
-    def current_request(self, value):  # noqa
-        # type: (Optional[Request]) -> None
-        thread_id = threading.current_thread().ident
-        assert thread_id is not None
-        self._current_request_lookup[thread_id] = value
-
-    def __getattr__(self, name):
-        # type: (str) -> Any
-        return getattr(self._chalice, name)
-
-    def __call__(self, *args, **kwargs):
-        # type: (Any, Any) -> Any
-        return self._chalice(*args, **kwargs)
+    def current_request(self, value):  # type: ignore
+        # type: (Request) -> None
+        self._THREAD_LOCAL.current_request = value


Test patch:
diff --git a/tests/functional/test_local.py b/tests/functional/test_local.py
index a66128b..7c4115c 100644
--- a/tests/functional/test_local.py
+++ b/tests/functional/test_local.py
@@ -4,6 +4,7 @@ import time
 import contextlib
 from threading import Thread
 from threading import Event
+from threading import Lock
 import json
 import subprocess
 from contextlib import contextmanager
@@ -14,7 +15,7 @@ from urllib3.util.retry import Retry
 from requests.adapters import HTTPAdapter
 
 from chalice import app
-from chalice.local import LocalDevServer
+from chalice.local import create_local_server
 from chalice.config import Config
 from chalice.utils import OSUtils
 
@@ -71,7 +72,7 @@ class ThreadedLocalServer(Thread):
         self._config = config
 
     def run(self):
-        self._server = LocalDevServer(
+        self._server = create_local_server(
             self._app_object, self._config, self._host, self._port)
         self._server_ready.set()
         self._server.serve_forever()
@@ -103,10 +104,10 @@ def http_session():
     session = requests.Session()
     retry = Retry(
         # How many connection-related errors to retry on.
-        connect=5,
-        # How many connection-related errors to retry on.
+        connect=10,
         # A backoff factor to apply between attempts after the second try.
         backoff_factor=2,
+        method_whitelist=['GET', 'POST', 'PUT'],
     )
     session.mount('http://', HTTPAdapter(max_retries=retry))
     return HTTPFetcher(session)
@@ -141,6 +142,9 @@ def local_server_factory(unused_tcp_port):
 def sample_app():
     demo = app.Chalice('demo-app')
 
+    thread_safety_check = []
+    lock = Lock()
+
     @demo.route('/', methods=['GET'])
     def index():
         return {'hello': 'world'}
@@ -149,9 +153,56 @@ def sample_app():
     def test_cors():
         return {'hello': 'world'}
 
+    @demo.route('/count', methods=['POST'])
+    def record_counter():
+        # An extra delay helps ensure we consistently fail if we're
+        # not thread safe.
+        time.sleep(0.001)
+        count = int(demo.current_request.json_body['counter'])
+        with lock:
+            thread_safety_check.append(count)
+
+    @demo.route('/count', methods=['GET'])
+    def get_record_counter():
+        return thread_safety_check[:]
+
     return demo
 
 
+def test_has_thread_safe_current_request(config, sample_app,
+                                         local_server_factory):
+    local_server, port = local_server_factory(sample_app, config)
+    local_server.wait_for_server_ready()
+
+    num_requests = 25
+    num_threads = 5
+
+    # The idea here is that each requests.post() has a unique 'counter'
+    # integer.  If the current request is thread safe we should see a number
+    # for each 0 - (num_requests * num_threads).  If it's not thread safe
+    # we'll see missing numbers and/or duplicates.
+    def make_requests(counter_start):
+        for i in range(counter_start * num_requests,
+                       (counter_start + 1) * num_requests):
+            # We're slowing the sending rate down a bit.  The threaded
+            # http server is good, but not great.  You can still overwhelm
+            # it pretty easily.
+            time.sleep(0.001)
+            requests.post(
+                'http://localhost:%s/count' % port, json={'counter': i})
+
+    threads = []
+    for i in range(num_threads):
+        threads.append(Thread(target=make_requests, args=(i,)))
+    for thread in threads:
+        thread.start()
+    for thread in threads:
+        thread.join()
+    response = requests.get('http://localhost:%s/count' % port)
+    assert len(response.json()) == len(range(num_requests * num_threads))
+    assert sorted(response.json()) == list(range(num_requests * num_threads))
+
+
 def test_can_accept_get_request(config, sample_app, local_server_factory):
     local_server, port = local_server_factory(sample_app, config)
     response = local_server.make_call(requests.get, '/', port)
diff --git a/tests/unit/test_local.py b/tests/unit/test_local.py
index 926049d..3ae0966 100644
--- a/tests/unit/test_local.py
+++ b/tests/unit/test_local.py
@@ -21,7 +21,6 @@ from chalice.local import NotAuthorizedError
 from chalice.local import ForbiddenError
 from chalice.local import InvalidAuthorizerError
 from chalice.local import LocalDevServer
-from chalice.local import LocalChalice
 
 
 AWS_REQUEST_ID_PATTERN = re.compile(
@@ -691,21 +690,6 @@ def test_can_provide_host_to_local_server(sample_app):
     assert dev_server.host == '0.0.0.0'
 
 
-def test_wraps_sample_app_with_local_chalice(sample_app):
-    dev_server = local.create_local_server(
-        sample_app, None, "127.0.0.1", 23456
-    )
-    assert isinstance(dev_server.app_object, LocalChalice)
-    assert dev_server.app_object._chalice is sample_app
-    assert dev_server.app_object.app_name is sample_app.app_name
-    dev_server.app_object.current_request = "foo"
-    assert dev_server.app_object.current_request == "foo"
-    assert (
-        dev_server.app_object.current_request
-        is not sample_app.current_request
-    )
-
-
 class TestLambdaContext(object):
     def test_can_get_remaining_time_once(self, lambda_context_args):
         time_source = FakeTimeSource([0, 5])