Merge lp:~cjwatson/launchpad/global-urlfetch-timeout into lp:launchpad

Proposed by Colin Watson on 2019-03-18
Status: Merged
Merged at revision: 18910
Proposed branch: lp:~cjwatson/launchpad/global-urlfetch-timeout
Merge into: lp:launchpad
Diff against target: 145 lines (+15/-16)
4 files modified
lib/lp/services/config/schema-lazr.conf (+4/-0)
lib/lp/services/mail/tests/test_signedmessage.py (+7/-1)
lib/lp/services/tests/test_timeout.py (+1/-13)
lib/lp/services/timeout.py (+3/-2)
To merge this branch: bzr merge lp:~cjwatson/launchpad/global-urlfetch-timeout
Reviewer Review Type Date Requested Status
William Grant code 2019-03-18 Approve on 2019-03-18
Review via email: mp+364669@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
William Grant (wgrant) :
review: Approve (code)
18911. By Colin Watson on 2019-03-18

Note that launchpad.urlfetch_timeout is overridden to something more specific in many contexts.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/services/config/schema-lazr.conf'
2--- lib/lp/services/config/schema-lazr.conf 2019-03-13 16:12:42 +0000
3+++ lib/lp/services/config/schema-lazr.conf 2019-03-18 12:23:22 +0000
4@@ -1104,6 +1104,10 @@
5 # datatype: string
6 feature_flags_endpoint:
7
8+# Default timeout for fetching remote URLs. Overridden to something more
9+# specific in many contexts, but this provides a fallback.
10+urlfetch_timeout: 30
11+
12 [launchpad_session]
13 # The database connection string.
14 # datatype: pgconnection
15
16=== modified file 'lib/lp/services/mail/tests/test_signedmessage.py'
17--- lib/lp/services/mail/tests/test_signedmessage.py 2018-04-22 23:30:37 +0000
18+++ lib/lp/services/mail/tests/test_signedmessage.py 2019-03-18 12:23:22 +0000
19@@ -1,4 +1,4 @@
20-# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
21+# Copyright 2009-2019 Canonical Ltd. This software is licensed under the
22 # GNU Affero General Public License version 3 (see the file LICENSE).
23
24 """Test the SignedMessage class."""
25@@ -32,6 +32,7 @@
26 import_public_test_keys,
27 import_secret_test_key,
28 )
29+from lp.testing.keyserver import KeyServerTac
30 from lp.testing.layers import DatabaseFunctionalLayer
31
32
33@@ -44,6 +45,9 @@
34 # Login with admin roles as we aren't testing access here.
35 TestCaseWithFactory.setUp(self, 'admin@canonical.com')
36 import_public_test_keys()
37+ # Ensure that tests will try to fetch keys from the keyserver.
38+ self.useFixture(KeyServerTac())
39+ getUtility(IGPGHandler).resetLocalState()
40
41 def test_unsigned_message(self):
42 # An unsigned message will not have a signature nor signed content,
43@@ -73,6 +77,7 @@
44 msg = self.factory.makeSignedMessage(
45 email_address=sender.preferredemail.email,
46 body=body, signing_context=signing_context)
47+ getUtility(IGPGHandler).resetLocalState()
48 self.assertFalse(msg.is_multipart())
49 return signed_message_from_string(msg.as_string())
50
51@@ -134,6 +139,7 @@
52 signature = gpghandler.signContent(
53 canonicalise_line_endings(body_text.as_string()),
54 key, 'test', gpgme.SIG_MODE_DETACH)
55+ gpghandler.resetLocalState()
56
57 attachment = Message()
58 attachment.set_payload(signature)
59
60=== modified file 'lib/lp/services/tests/test_timeout.py'
61--- lib/lp/services/tests/test_timeout.py 2018-07-13 12:48:19 +0000
62+++ lib/lp/services/tests/test_timeout.py 2019-03-18 12:23:22 +0000
63@@ -1,4 +1,4 @@
64-# Copyright 2012-2018 Canonical Ltd. This software is licensed under the
65+# Copyright 2012-2019 Canonical Ltd. This software is licensed under the
66 # GNU Affero General Public License version 3 (see the file LICENSE).
67
68 """timeout.py tests.
69@@ -370,8 +370,6 @@
70 def test_urlfetch_no_proxy_by_default(self):
71 """urlfetch does not use a proxy by default."""
72 self.pushConfig('launchpad', http_proxy='http://proxy.example:3128/')
73- set_default_timeout_function(lambda: 1)
74- self.addCleanup(set_default_timeout_function, None)
75 fake_send = FakeMethod(result=Response())
76 self.useFixture(
77 MonkeyPatch('requests.adapters.HTTPAdapter.send', fake_send))
78@@ -382,8 +380,6 @@
79 """urlfetch uses proxies if explicitly requested."""
80 proxy = 'http://proxy.example:3128/'
81 self.pushConfig('launchpad', http_proxy=proxy)
82- set_default_timeout_function(lambda: 1)
83- self.addCleanup(set_default_timeout_function, None)
84 fake_send = FakeMethod(result=Response())
85 self.useFixture(
86 MonkeyPatch('requests.adapters.HTTPAdapter.send', fake_send))
87@@ -394,8 +390,6 @@
88
89 def test_urlfetch_does_not_support_ftp_urls_by_default(self):
90 """urlfetch() does not support ftp urls by default."""
91- set_default_timeout_function(lambda: 1)
92- self.addCleanup(set_default_timeout_function, None)
93 url = 'ftp://localhost/'
94 e = self.assertRaises(InvalidSchema, urlfetch, url)
95 self.assertEqual(
96@@ -420,8 +414,6 @@
97 self.pushConfig('launchpad', http_proxy=http_server_url)
98 t = threading.Thread(target=success_result)
99 t.start()
100- set_default_timeout_function(lambda: 1)
101- self.addCleanup(set_default_timeout_function, None)
102 response = urlfetch(
103 'ftp://example.com/', use_proxy=True, allow_ftp=True)
104 self.assertThat(response, MatchesStructure(
105@@ -432,8 +424,6 @@
106
107 def test_urlfetch_does_not_support_file_urls_by_default(self):
108 """urlfetch() does not support file urls by default."""
109- set_default_timeout_function(lambda: 1)
110- self.addCleanup(set_default_timeout_function, None)
111 test_path = self.useFixture(TempDir()).join('file')
112 write_file(test_path, '')
113 url = 'file://' + test_path
114@@ -443,8 +433,6 @@
115
116 def test_urlfetch_supports_file_urls_if_allow_file(self):
117 """urlfetch() supports file urls if explicitly asked to do so."""
118- set_default_timeout_function(lambda: 1)
119- self.addCleanup(set_default_timeout_function, None)
120 test_path = self.useFixture(TempDir()).join('file')
121 write_file(test_path, 'Success.')
122 url = 'file://' + test_path
123
124=== modified file 'lib/lp/services/timeout.py'
125--- lib/lp/services/timeout.py 2018-07-26 14:19:38 +0000
126+++ lib/lp/services/timeout.py 2019-03-18 12:23:22 +0000
127@@ -1,4 +1,4 @@
128-# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
129+# Copyright 2009-2019 Canonical Ltd. This software is licensed under the
130 # GNU Affero General Public License version 3 (see the file LICENSE).
131
132 """Helpers to time out external operations."""
133@@ -396,7 +396,8 @@
134
135 def urlfetch(url, **request_kwargs):
136 """Wrapper for `requests.get()` that times out."""
137- return URLFetcher().fetch(url, **request_kwargs)
138+ with default_timeout(config.launchpad.urlfetch_timeout):
139+ return URLFetcher().fetch(url, **request_kwargs)
140
141
142 class TransportWithTimeout(Transport):
143
144=== added symlink 'lib/lp/testing/keyserver/tests/keys/0xDB25975602BA5EF6.get'
145=== target is u'0xA419AE861E88BC9E04B9C26FBA2B9389DFD20543.get'