launchpad-reviewers team mailing list archive

[Tom Wardill <tom.wardill@xxxxxxxxxxxxx>]

Tom Wardill has proposed merging ~twom/launchpad:thread-state-not-in-state into launchpad:master.

Commit message:
Move timeout out of local thread state

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~twom/launchpad/+git/launchpad/+merge/389327

Keeping the timeout in the thread local (created in __init__) meant that if the _checkError method was called from another thread, the timeout value was missing.
Move it to a class attribute and add a test to ensure it's fixed.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~twom/launchpad:thread-state-not-in-state into launchpad:master.

diff --git a/lib/lp/services/librarian/client.py b/lib/lp/services/librarian/client.py
index 737d9e4..219d80d 100644
--- a/lib/lp/services/librarian/client.py
+++ b/lib/lp/services/librarian/client.py
@@ -83,6 +83,8 @@ def compose_url(base_url, alias_path):
 class FileUploadClient:
     """Simple blocking client for uploading to the librarian."""
 
+    s_poll_timeout = 0
+
     def __init__(self):
         # This class is registered as a utility, which means an instance of
         # it will be shared between threads. The easiest way of making this
@@ -115,7 +117,7 @@ class FileUploadClient:
         del self.state.f
 
     def _checkError(self):
-        poll_result = self.state.s_poll.poll(0)
+        poll_result = self.state.s_poll.poll(self.s_poll_timeout)
         if poll_result:
             fileno, event = poll_result[0]
             # Accepts any event that contains input data. Even if we
diff --git a/lib/lp/services/librarian/tests/test_client.py b/lib/lp/services/librarian/tests/test_client.py
index e00a86e..9c671cb 100644
--- a/lib/lp/services/librarian/tests/test_client.py
+++ b/lib/lp/services/librarian/tests/test_client.py
@@ -46,6 +46,27 @@ from lp.testing.layers import (
 from lp.testing.views import create_webservice_error_view
 
 
+class PropagatingThread(threading.Thread):
+    """Thread class that propagates errors to the parent."""
+    # https://stackoverflow.com/a/31614591
+    def run(self):
+        self.exc = None
+        try:
+            if hasattr(self, '_Thread__target'):
+                # Thread uses name mangling prior to Python 3.
+                self.ret = self._Thread__target(
+                    *self._Thread__args, **self._Thread__kwargs)
+            else:
+                self.ret = self._target(*self._args, **self._kwargs)
+        except BaseException as e:
+            self.exc = e
+
+    def join(self):
+        super(PropagatingThread, self).join()
+        if self.exc:
+            raise self.exc
+
+
 class InstrumentedLibrarianClient(LibrarianClient):
 
     def __init__(self, *args, **kwargs):
@@ -252,6 +273,16 @@ class LibrarianClientTestCase(TestCase):
             'librarian', upload_host=upload_host, upload_port=upload_port)
 
         client = LibrarianClient()
+        # Artificially increases timeout to avoid race condition.
+        # The fake server is running in another thread, and we are sure it
+        # will (eventually) reply with an error message. So, let's just wait
+        # until that message arrives.
+        client.s_poll_timeout = 120
+
+        # Please, note the mismatch between file size (7) and its actual size
+        # of the content (6). This is intentional, to make sure we are raising
+        # the error coming from the fake server (and not the local check
+        # right after, while uploading the file).
         self.assertRaisesRegex(
             UploadFailed, 'Server said early: STORE 7 sample.txt',
             client.addFile, 'sample.txt', 7, StringIO('sample'), 'text/plain')
@@ -467,6 +498,15 @@ class LibrarianClientTestCase(TestCase):
 
         client_module._File = _File
 
+    def test_thread_state_FileUploadClient(self):
+        client = InstrumentedLibrarianClient()
+        th = PropagatingThread(
+            target=client.addFile,
+            args=('sample.txt', 6, StringIO('sample'), 'text/plain'))
+        th.start()
+        th.join()
+        self.assertEqual(5, client.check_error_calls)
+
 
 class TestWebServiceErrors(unittest.TestCase):
     """ Test that errors are correctly mapped to HTTP status codes."""