Merge lp:~cjwatson/launchpad/git-ref-tolerate-slow-commit-info into lp:launchpad

Proposed by Colin Watson on 2018-11-29
Status: Merged
Merged at revision: 18847
Proposed branch: lp:~cjwatson/launchpad/git-ref-tolerate-slow-commit-info
Merge into: lp:launchpad
Diff against target: 286 lines (+101/-22)
5 files modified
lib/lp/code/browser/gitref.py (+3/-1)
lib/lp/code/browser/tests/test_gitref.py (+45/-4)
lib/lp/code/interfaces/gitref.py (+6/-2)
lib/lp/code/model/gitref.py (+42/-15)
lib/lp/code/templates/git-macros.pt (+5/-0)
To merge this branch: bzr merge lp:~cjwatson/launchpad/git-ref-tolerate-slow-commit-info
Reviewer Review Type Date Requested Status
William Grant code 2018-11-29 Approve on 2018-12-20
Review via email: mp+359827@code.launchpad.net

Commit message

Tolerate backend timeouts while fetching commit information for GitRef:+index.

To post a comment you must log in.
William Grant (wgrant) :
review: Approve (code)
18828. By Colin Watson on 2018-12-20

Pull handle_timeout=True out to GitRefView.

Colin Watson (cjwatson) :

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/browser/gitref.py'
2--- lib/lp/code/browser/gitref.py 2018-08-30 14:54:37 +0000
3+++ lib/lp/code/browser/gitref.py 2018-12-20 11:14:34 +0000
4@@ -49,6 +49,7 @@
5 from lp.code.interfaces.gitrepository import IGitRepositorySet
6 from lp.services.helpers import english_list
7 from lp.services.propertycache import cachedproperty
8+from lp.services.scripts import log
9 from lp.services.webapp import (
10 canonical_url,
11 ContextMenu,
12@@ -180,7 +181,8 @@
13 @cachedproperty
14 def commit_infos(self):
15 return self.context.getLatestCommits(
16- extended_details=True, user=self.user)
17+ extended_details=True, user=self.user, handle_timeout=True,
18+ logger=log)
19
20 @property
21 def recipes_link(self):
22
23=== modified file 'lib/lp/code/browser/tests/test_gitref.py'
24--- lib/lp/code/browser/tests/test_gitref.py 2018-09-27 13:50:06 +0000
25+++ lib/lp/code/browser/tests/test_gitref.py 2018-12-20 11:14:34 +0000
26@@ -11,10 +11,14 @@
27 import hashlib
28 import re
29
30+from fixtures import FakeLogger
31 import pytz
32 import soupmatchers
33 from storm.store import Store
34-from testtools.matchers import Equals
35+from testtools.matchers import (
36+ Equals,
37+ Not,
38+ )
39 from zope.component import getUtility
40 from zope.security.proxy import removeSecurityProxy
41
42@@ -23,6 +27,7 @@
43 from lp.code.tests.helpers import GitHostingFixture
44 from lp.services.beautifulsoup import BeautifulSoup
45 from lp.services.job.runner import JobRunner
46+from lp.services.timeout import TimeoutError
47 from lp.services.utils import seconds_since_epoch
48 from lp.services.webapp.publisher import canonical_url
49 from lp.testing import (
50@@ -82,6 +87,14 @@
51 canonical_url(ref))
52
53
54+class MissingCommitsNote(soupmatchers.Tag):
55+
56+ def __init__(self):
57+ super(MissingCommitsNote, self).__init__(
58+ "missing commits note", "div",
59+ text="Some recent commit information could not be fetched.")
60+
61+
62 class TestGitRefView(BrowserTestCase):
63
64 layer = LaunchpadFunctionalLayer
65@@ -186,11 +199,12 @@
66 self.hosting_fixture.getLog.result = list(reversed(log))
67 self.scanRef(ref, log[-1])
68 view = create_initialized_view(ref, "+index")
69+ contents = view()
70 expected_texts = list(reversed([
71 "%.7s...\nby\n%s\non 2015-01-%02d" % (
72 log[i]["sha1"], log[i]["author"]["name"], i + 1)
73 for i in range(5)]))
74- details = find_tags_by_class(view(), "commit-details")
75+ details = find_tags_by_class(contents, "commit-details")
76 self.assertEqual(
77 expected_texts, [extract_text(detail) for detail in details])
78 expected_urls = list(reversed([
79@@ -199,6 +213,8 @@
80 for i in range(5)]))
81 self.assertEqual(
82 expected_urls, [detail.a["href"] for detail in details])
83+ self.assertThat(
84+ contents, Not(soupmatchers.HTMLContains(MissingCommitsNote())))
85
86 def test_recent_commits_with_merge(self):
87 [ref] = self.factory.makeGitRefs(paths=["refs/heads/branch"])
88@@ -211,7 +227,8 @@
89 self.scanRef(mp.merge_source, merged_tip)
90 mp.markAsMerged(merged_revision_id=log[0]["sha1"])
91 view = create_initialized_view(ref, "+index")
92- soup = BeautifulSoup(view())
93+ contents = view()
94+ soup = BeautifulSoup(contents)
95 details = soup.findAll(
96 attrs={"class": re.compile(r"commit-details|commit-comment")})
97 expected_texts = list(reversed([
98@@ -225,6 +242,8 @@
99 self.assertEqual(
100 [canonical_url(mp), canonical_url(mp.merge_source)],
101 [link["href"] for link in details[5].findAll("a")])
102+ self.assertThat(
103+ contents, Not(soupmatchers.HTMLContains(MissingCommitsNote())))
104
105 def test_recent_commits_with_merge_from_deleted_ref(self):
106 [ref] = self.factory.makeGitRefs(paths=["refs/heads/branch"])
107@@ -238,7 +257,8 @@
108 mp.markAsMerged(merged_revision_id=log[0]["sha1"])
109 mp.source_git_repository.removeRefs([mp.source_git_path])
110 view = create_initialized_view(ref, "+index")
111- soup = BeautifulSoup(view())
112+ contents = view()
113+ soup = BeautifulSoup(contents)
114 details = soup.findAll(
115 attrs={"class": re.compile(r"commit-details|commit-comment")})
116 expected_texts = list(reversed([
117@@ -252,6 +272,8 @@
118 self.assertEqual(
119 [canonical_url(mp)],
120 [link["href"] for link in details[5].findAll("a")])
121+ self.assertThat(
122+ contents, Not(soupmatchers.HTMLContains(MissingCommitsNote())))
123
124 def _test_all_commits_link(self, branch_name, encoded_branch_name=None):
125 if encoded_branch_name is None:
126@@ -312,3 +334,22 @@
127 with StormStatementRecorder() as recorder:
128 view.landing_targets
129 self.assertThat(recorder, HasQueryCount(Equals(12)))
130+
131+ def test_timeout(self):
132+ # The page renders even if fetching commits times out.
133+ self.useFixture(FakeLogger())
134+ [ref] = self.factory.makeGitRefs()
135+ log = self.makeCommitLog()
136+ self.scanRef(ref, log[-1])
137+ self.hosting_fixture.getLog.failure = TimeoutError
138+ view = create_initialized_view(ref, "+index")
139+ contents = view()
140+ soup = BeautifulSoup(contents)
141+ details = soup.findAll(
142+ attrs={"class": re.compile(r"commit-details|commit-comment")})
143+ expected_text = "%.7s...\nby\n%s\non 2015-01-%02d" % (
144+ log[-1]["sha1"], log[-1]["author"]["name"], len(log))
145+ self.assertEqual(
146+ [expected_text], [extract_text(detail) for detail in details])
147+ self.assertThat(
148+ contents, soupmatchers.HTMLContains(MissingCommitsNote()))
149
150=== modified file 'lib/lp/code/interfaces/gitref.py'
151--- lib/lp/code/interfaces/gitref.py 2018-10-25 17:52:43 +0000
152+++ lib/lp/code/interfaces/gitref.py 2018-12-20 11:14:34 +0000
153@@ -364,7 +364,8 @@
154 "yet been scanned.")
155
156 def getCommits(start, limit=None, stop=None, union_repository=None,
157- start_date=None, end_date=None, logger=None):
158+ start_date=None, end_date=None, handle_timeout=False,
159+ logger=None):
160 """Get commit information from this reference.
161
162 :param start: The commit to start listing from.
163@@ -374,11 +375,14 @@
164 this repository as well (particularly useful with `stop`).
165 :param start_date: If not None, ignore commits before this date.
166 :param end_date: If not None, ignore commits after this date.
167+ :param handle_timeout: If True and the backend request times out,
168+ synthesise commit information from what we have in the database.
169 :param logger: An optional logger.
170 :return: An iterable of commit information dicts.
171 """
172
173- def getLatestCommits(quantity=10):
174+ def getLatestCommits(quantity=10, extended_details=False, user=None,
175+ handle_timeout=False, logger=None):
176 """Return a specific number of the latest commits in this ref."""
177
178 has_commits = Attribute("Whether this reference has any commits.")
179
180=== modified file 'lib/lp/code/model/gitref.py'
181--- lib/lp/code/model/gitref.py 2018-12-20 10:03:26 +0000
182+++ lib/lp/code/model/gitref.py 2018-12-20 11:14:34 +0000
183@@ -83,7 +83,11 @@
184 from lp.services.database.stormbase import StormBase
185 from lp.services.features import getFeatureFlag
186 from lp.services.memcache.interfaces import IMemcacheClient
187-from lp.services.timeout import urlfetch
188+from lp.services.timeout import (
189+ reduced_timeout,
190+ TimeoutError,
191+ urlfetch,
192+ )
193 from lp.services.utils import seconds_since_epoch
194 from lp.services.webapp.interfaces import ILaunchBag
195
196@@ -336,9 +340,10 @@
197 try:
198 log = json.loads(cached_log)
199 except Exception:
200- logger.exception(
201- "Cannot load cached log information for %s:%s; "
202- "deleting" % (path, start))
203+ if logger is not None:
204+ logger.exception(
205+ "Cannot load cached log information for %s:%s; "
206+ "deleting" % (path, start))
207 memcache_client.delete(memcache_key)
208 if log is None:
209 if enable_hosting:
210@@ -367,19 +372,38 @@
211 return log
212
213 def getCommits(self, start, limit=None, stop=None, union_repository=None,
214- start_date=None, end_date=None, logger=None):
215+ start_date=None, end_date=None, handle_timeout=False,
216+ logger=None):
217 # Circular import.
218 from lp.code.model.gitrepository import parse_git_commits
219
220- log = self._getLog(
221- start, limit=limit, stop=stop, union_repository=union_repository,
222- logger=logger)
223- parsed_commits = parse_git_commits(log)
224+ with reduced_timeout(1.0 if handle_timeout else 0.0):
225+ try:
226+ log = self._getLog(
227+ start, limit=limit, stop=stop,
228+ union_repository=union_repository, logger=logger)
229+ commit_oids = [
230+ commit["sha1"] for commit in log if "sha1" in commit]
231+ parsed_commits = parse_git_commits(log)
232+ except TimeoutError:
233+ if not handle_timeout:
234+ raise
235+ if logger is not None:
236+ logger.exception(
237+ "Timeout fetching commits for %s" % self.identity)
238+ # Synthesise a response based on what we have in the database.
239+ commit_oids = [self.commit_sha1]
240+ tip = {"sha1": self.commit_sha1, "synthetic": True}
241+ optional_fields = (
242+ "author", "author_date", "committer", "committer_date",
243+ "commit_message")
244+ for field in optional_fields:
245+ if getattr(self, field) is not None:
246+ tip[field] = getattr(self, field)
247+ parsed_commits = {self.commit_sha1: tip}
248 commits = []
249- for commit in log:
250- if "sha1" not in commit:
251- continue
252- parsed_commit = parsed_commits[commit["sha1"]]
253+ for commit_oid in commit_oids:
254+ parsed_commit = parsed_commits[commit_oid]
255 author_date = parsed_commit.get("author_date")
256 if start_date is not None:
257 if author_date is None or author_date < start_date:
258@@ -390,8 +414,11 @@
259 commits.append(parsed_commit)
260 return commits
261
262- def getLatestCommits(self, quantity=10, extended_details=False, user=None):
263- commits = self.getCommits(self.commit_sha1, limit=quantity)
264+ def getLatestCommits(self, quantity=10, extended_details=False, user=None,
265+ handle_timeout=False, logger=None):
266+ commits = self.getCommits(
267+ self.commit_sha1, limit=quantity, handle_timeout=handle_timeout,
268+ logger=logger)
269 if extended_details:
270 # XXX cjwatson 2016-05-09: Add support for linked bugtasks once
271 # those are supported for Git.
272
273=== modified file 'lib/lp/code/templates/git-macros.pt'
274--- lib/lp/code/templates/git-macros.pt 2018-08-24 16:52:27 +0000
275+++ lib/lp/code/templates/git-macros.pt 2018-12-20 11:14:34 +0000
276@@ -135,6 +135,11 @@
277 </tal:diff-expander>
278 </tal:ajax-revision-diffs>
279 </dl>
280+ <tal:synthetic
281+ condition="python: any(commit_info.get('synthetic')
282+ for commit_info in commit_infos)">
283+ <div>Some recent commit information could not be fetched.</div>
284+ </tal:synthetic>
285
286 </metal:ref-commits>
287