launchpad-reviewers team mailing list archive

[Yuliy Schwartzburg <mp+478591@xxxxxxxxxxxxxxxxxx>]

Yuliy Schwartzburg has proposed merging ~ikoruk/launchpad:fix-urllib3 into launchpad:master.

Commit message:
Fix timeout class to work properly with urllib3>=1.26.16

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

The `lp.services.tests.test_timeout.TestTimeout.test_urlfetch_timeout_after_listen` test was failing due to `urllib3` being updated to be thread-safe. Therefore, instead, we should use a process so we can kill it explicitly.

We also introduce the config timeout explicitly to the request so even if it's not killed, it will not hang.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ikoruk/launchpad:fix-urllib3 into launchpad:master.

diff --git a/lib/lp/services/timeout.py b/lib/lp/services/timeout.py
index 99a4847..251e51d 100644
--- a/lib/lp/services/timeout.py
+++ b/lib/lp/services/timeout.py
@@ -20,7 +20,8 @@ import re
 import socket
 import sys
 from contextlib import contextmanager
-from threading import Lock, Thread
+from threading import Lock
+from multiprocessing import Process
 from xmlrpc.client import Transport
 
 from requests import HTTPError, Session
@@ -123,7 +124,7 @@ class TimeoutError(Exception):
     """Exception raised when a function doesn't complete within time."""
 
 
-class ThreadCapturingResult(Thread):
+class ProcessCapturingResult(Process):
     """Thread subclass that saves the return value of its target.
 
     It also saves exceptions raised when invoking the target.
@@ -199,7 +200,7 @@ class with_timeout:
                 else:
                     self.cleanup()
                 # Collect cleaned-up worker thread.
-                t.join()
+                t.terminate()
 
         def call_with_timeout(*args, **kwargs):
             # Ensure that we have a timeout before we start the thread
@@ -211,7 +212,7 @@ class with_timeout:
                     timeout = timeout(args[0])
                 else:
                     timeout = timeout()
-            t = ThreadCapturingResult(f, args, kwargs)
+            t = ProcessCapturingResult(f, args, kwargs)
             t.start()
             try:
                 t.join(timeout)
@@ -419,7 +420,9 @@ class URLFetcher:
 def urlfetch(url, **request_kwargs):
     """Wrapper for `requests.get()` that times out."""
     with default_timeout(config.launchpad.urlfetch_timeout):
-        return URLFetcher().fetch(url, **request_kwargs)
+        return URLFetcher().fetch(
+            url, timeout=config.launchpad.urlfetch_timeout, **request_kwargs
+        )
 
 
 class TransportWithTimeout(Transport):