launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/job-default-timeout into lp:launchpad.

Commit message:
Set a default timeout function when running jobs.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1578961 in Launchpad itself: "Newly pushed git branches are not shown in launchpad web"
  https://bugs.launchpad.net/launchpad/+bug/1578961

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/job-default-timeout/+merge/293990

Set a default timeout function when running jobs.

This makes GitHostingClient usable from jobs again following the conversion to urlfetch.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/job-default-timeout into lp:launchpad.

=== modified file 'lib/lp/code/model/tests/test_githosting.py'
--- lib/lp/code/model/tests/test_githosting.py	2016-05-03 18:42:18 +0000
+++ lib/lp/code/model/tests/test_githosting.py	2016-05-06 09:37:15 +0000
@@ -20,6 +20,7 @@
     )
 from testtools.matchers import MatchesStructure
 from zope.component import getUtility
+from zope.interface import implementer
 from zope.security.proxy import removeSecurityProxy
 
 from lp.code.errors import (
@@ -28,14 +29,27 @@
     GitRepositoryScanFault,
     )
 from lp.code.interfaces.githosting import IGitHostingClient
+from lp.services.job.interfaces.job import (
+    IRunnableJob,
+    JobStatus,
+    )
+from lp.services.job.model.job import Job
+from lp.services.job.runner import (
+    BaseRunnableJob,
+    JobRunner,
+    )
+from lp.services.timeout import (
+    get_default_timeout_function,
+    set_default_timeout_function,
+    )
 from lp.services.webapp.url import urlappend
 from lp.testing import TestCase
-from lp.testing.layers import LaunchpadZopelessLayer
+from lp.testing.layers import ZopelessDatabaseLayer
 
 
 class TestGitHostingClient(TestCase):
 
-    layer = LaunchpadZopelessLayer
+    layer = ZopelessDatabaseLayer
 
     def setUp(self):
         super(TestGitHostingClient, self).setUp()
@@ -44,7 +58,8 @@
         self.request = None
 
     @contextmanager
-    def mockRequests(self, status_code=200, content=b""):
+    def mockRequests(self, status_code=200, content=b"",
+                     set_default_timeout=True):
         @all_requests
         def handler(url, request):
             self.assertIsNone(self.request)
@@ -52,7 +67,13 @@
             return {"status_code": status_code, "content": content}
 
         with HTTMock(handler):
-            yield
+            original_timeout_function = get_default_timeout_function()
+            if set_default_timeout:
+                set_default_timeout_function(lambda: 60.0)
+            try:
+                yield
+            finally:
+                set_default_timeout_function(original_timeout_function)
 
     def assertRequest(self, url_suffix, json_data=None, **kwargs):
         self.assertThat(self.request, MatchesStructure.byEquality(
@@ -249,3 +270,24 @@
                 "Failed to get file from Git repository: Unexpected size"
                 " (256 vs 0)",
                 self.client.getBlob, "123", "dir/path/file/name")
+
+    def test_works_in_job(self):
+        # `GitHostingClient` is usable from a running job.
+        @implementer(IRunnableJob)
+        class GetRefsJob(BaseRunnableJob):
+            def __init__(self, testcase):
+                super(GetRefsJob, self).__init__()
+                self.job = Job()
+                self.testcase = testcase
+
+            def run(self):
+                with self.testcase.mockRequests(
+                        content=b'{"refs/heads/master": {}}',
+                        set_default_timeout=False):
+                    self.refs = self.testcase.client.getRefs("123")
+
+        job = GetRefsJob(self)
+        JobRunner([job]).runAll()
+        self.assertEqual(JobStatus.COMPLETED, job.job.status)
+        self.assertEqual({"refs/heads/master": {}}, job.refs)
+        self.assertRequest("repo/123/refs", method="GET")

=== modified file 'lib/lp/services/job/runner.py'
--- lib/lp/services/job/runner.py	2015-08-04 00:19:36 +0000
+++ lib/lp/services/job/runner.py	2016-05-06 09:37:15 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Facilities for running Jobs."""
@@ -75,6 +75,10 @@
     MailController,
     set_immediate_mail_delivery,
     )
+from lp.services.timeout import (
+    get_default_timeout_function,
+    set_default_timeout_function,
+    )
 from lp.services.twistedsupport import run_reactor
 from lp.services.webapp import errorlog
 
@@ -299,7 +303,13 @@
         return True
 
     def runJob(self, job, fallback):
-        super(BaseJobRunner, self).runJob(IRunnableJob(job), fallback)
+        original_timeout_function = get_default_timeout_function()
+        if job.lease_expires is not None:
+            set_default_timeout_function(lambda: job.getTimeout())
+        try:
+            super(BaseJobRunner, self).runJob(IRunnableJob(job), fallback)
+        finally:
+            set_default_timeout_function(original_timeout_function)
 
     def userErrorTypes(self, job):
         return removeSecurityProxy(job).user_error_types

=== modified file 'lib/lp/services/job/tests/test_runner.py'
--- lib/lp/services/job/tests/test_runner.py	2015-07-28 00:28:51 +0000
+++ lib/lp/services/job/tests/test_runner.py	2016-05-06 09:37:15 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for job-running facilities."""
@@ -42,6 +42,10 @@
     TwistedJobRunner,
     )
 from lp.services.log.logger import BufferLogger
+from lp.services.timeout import (
+    get_default_timeout_function,
+    set_default_timeout_function,
+    )
 from lp.services.webapp import errorlog
 from lp.testing import (
     TestCaseWithFactory,
@@ -360,6 +364,26 @@
         self.assertNotIn(job, runner.completed_jobs)
         self.assertIn(job, runner.incomplete_jobs)
 
+    def test_runJob_sets_default_timeout_function(self):
+        """runJob sets a default timeout function for urlfetch."""
+        class RecordDefaultTimeoutJob(NullJob):
+            def __init__(self):
+                super(RecordDefaultTimeoutJob, self).__init__("")
+
+            def run(self):
+                self.default_timeout = get_default_timeout_function()()
+
+        original_timeout_function = get_default_timeout_function()
+        set_default_timeout_function(None)
+        try:
+            job = RecordDefaultTimeoutJob()
+            job.job.acquireLease()
+            JobRunner([job]).runJob(job, None)
+            self.assertEqual(JobStatus.COMPLETED, job.job.status)
+            self.assertThat(job.default_timeout, GreaterThan(0))
+        finally:
+            set_default_timeout_function(original_timeout_function)
+
     def test_runJobHandleErrors_oops_generated_notify_fails(self):
         """A second oops is logged if the notification of the oops fails."""
         job = RaisingJobRaisingNotifyOops('boom')