launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/global-urlfetch-timeout into lp:launchpad.

Commit message:
Give urlfetch a default timeout, fixing a regression in process-mail.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1820552 in Launchpad itself: "Some OpenPGP verification callsites crash because they have no timeout"
  https://bugs.launchpad.net/launchpad/+bug/1820552

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/global-urlfetch-timeout/+merge/364669

The default timeout assertion has been driving me nuts ever since I started converting more things to urlfetch, so let's make urlfetch have a reasonable default for scripts.  Contexts with a timeout budget need to override this, of course, but they generally already do.

In theory this should obviate a number of more specific timeout configuration items, but I decided to leave those alone in case they need isolated overrides for some reason; we can just avoid adding more unless they're specifically needed.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/global-urlfetch-timeout into lp:launchpad.

=== modified file 'lib/lp/services/config/schema-lazr.conf'
--- lib/lp/services/config/schema-lazr.conf	2019-03-13 16:12:42 +0000
+++ lib/lp/services/config/schema-lazr.conf	2019-03-18 12:14:41 +0000
@@ -1104,6 +1104,9 @@
 # datatype: string
 feature_flags_endpoint:
 
+# Default timeout for fetching remote URLs.
+urlfetch_timeout: 30
+
 [launchpad_session]
 # The database connection string.
 # datatype: pgconnection

=== modified file 'lib/lp/services/mail/tests/test_signedmessage.py'
--- lib/lp/services/mail/tests/test_signedmessage.py	2018-04-22 23:30:37 +0000
+++ lib/lp/services/mail/tests/test_signedmessage.py	2019-03-18 12:14:41 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test the SignedMessage class."""
@@ -32,6 +32,7 @@
     import_public_test_keys,
     import_secret_test_key,
     )
+from lp.testing.keyserver import KeyServerTac
 from lp.testing.layers import DatabaseFunctionalLayer
 
 
@@ -44,6 +45,9 @@
         # Login with admin roles as we aren't testing access here.
         TestCaseWithFactory.setUp(self, 'admin@xxxxxxxxxxxxx')
         import_public_test_keys()
+        # Ensure that tests will try to fetch keys from the keyserver.
+        self.useFixture(KeyServerTac())
+        getUtility(IGPGHandler).resetLocalState()
 
     def test_unsigned_message(self):
         # An unsigned message will not have a signature nor signed content,
@@ -73,6 +77,7 @@
         msg = self.factory.makeSignedMessage(
             email_address=sender.preferredemail.email,
             body=body, signing_context=signing_context)
+        getUtility(IGPGHandler).resetLocalState()
         self.assertFalse(msg.is_multipart())
         return signed_message_from_string(msg.as_string())
 
@@ -134,6 +139,7 @@
         signature = gpghandler.signContent(
             canonicalise_line_endings(body_text.as_string()),
             key, 'test', gpgme.SIG_MODE_DETACH)
+        gpghandler.resetLocalState()
 
         attachment = Message()
         attachment.set_payload(signature)

=== modified file 'lib/lp/services/tests/test_timeout.py'
--- lib/lp/services/tests/test_timeout.py	2018-07-13 12:48:19 +0000
+++ lib/lp/services/tests/test_timeout.py	2019-03-18 12:14:41 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """timeout.py tests.
@@ -370,8 +370,6 @@
     def test_urlfetch_no_proxy_by_default(self):
         """urlfetch does not use a proxy by default."""
         self.pushConfig('launchpad', http_proxy='http://proxy.example:3128/')
-        set_default_timeout_function(lambda: 1)
-        self.addCleanup(set_default_timeout_function, None)
         fake_send = FakeMethod(result=Response())
         self.useFixture(
             MonkeyPatch('requests.adapters.HTTPAdapter.send', fake_send))
@@ -382,8 +380,6 @@
         """urlfetch uses proxies if explicitly requested."""
         proxy = 'http://proxy.example:3128/'
         self.pushConfig('launchpad', http_proxy=proxy)
-        set_default_timeout_function(lambda: 1)
-        self.addCleanup(set_default_timeout_function, None)
         fake_send = FakeMethod(result=Response())
         self.useFixture(
             MonkeyPatch('requests.adapters.HTTPAdapter.send', fake_send))
@@ -394,8 +390,6 @@
 
     def test_urlfetch_does_not_support_ftp_urls_by_default(self):
         """urlfetch() does not support ftp urls by default."""
-        set_default_timeout_function(lambda: 1)
-        self.addCleanup(set_default_timeout_function, None)
         url = 'ftp://localhost/'
         e = self.assertRaises(InvalidSchema, urlfetch, url)
         self.assertEqual(
@@ -420,8 +414,6 @@
         self.pushConfig('launchpad', http_proxy=http_server_url)
         t = threading.Thread(target=success_result)
         t.start()
-        set_default_timeout_function(lambda: 1)
-        self.addCleanup(set_default_timeout_function, None)
         response = urlfetch(
             'ftp://example.com/', use_proxy=True, allow_ftp=True)
         self.assertThat(response, MatchesStructure(
@@ -432,8 +424,6 @@
 
     def test_urlfetch_does_not_support_file_urls_by_default(self):
         """urlfetch() does not support file urls by default."""
-        set_default_timeout_function(lambda: 1)
-        self.addCleanup(set_default_timeout_function, None)
         test_path = self.useFixture(TempDir()).join('file')
         write_file(test_path, '')
         url = 'file://' + test_path
@@ -443,8 +433,6 @@
 
     def test_urlfetch_supports_file_urls_if_allow_file(self):
         """urlfetch() supports file urls if explicitly asked to do so."""
-        set_default_timeout_function(lambda: 1)
-        self.addCleanup(set_default_timeout_function, None)
         test_path = self.useFixture(TempDir()).join('file')
         write_file(test_path, 'Success.')
         url = 'file://' + test_path

=== modified file 'lib/lp/services/timeout.py'
--- lib/lp/services/timeout.py	2018-07-26 14:19:38 +0000
+++ lib/lp/services/timeout.py	2019-03-18 12:14:41 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Helpers to time out external operations."""
@@ -396,7 +396,8 @@
 
 def urlfetch(url, **request_kwargs):
     """Wrapper for `requests.get()` that times out."""
-    return URLFetcher().fetch(url, **request_kwargs)
+    with default_timeout(config.launchpad.urlfetch_timeout):
+        return URLFetcher().fetch(url, **request_kwargs)
 
 
 class TransportWithTimeout(Transport):

=== added symlink 'lib/lp/testing/keyserver/tests/keys/0xDB25975602BA5EF6.get'
=== target is u'0xA419AE861E88BC9E04B9C26FBA2B9389DFD20543.get'