Launchpad itself

Merge lp:~jml/launchpad/until-no-eintr into lp:launchpad

  1. until-no-eintr
  2. Merge into devel
Proposed by Jonathan Lange
Status: Merged
Approved by: Julian Edwards
Approved revision: no longer in the source branch.
Merged at revision: 11398
Proposed branch: lp:~jml/launchpad/until-no-eintr
Merge into: lp:launchpad
Diff against target: 273 lines (+165/-34)
3 files modified
lib/lp/buildmaster/model/builder.py (+29/-32)
lib/lp/services/osutils.py (+33/-1)
lib/lp/services/tests/test_osutils.py (+103/-1)
To merge this branch: bzr merge lp:~jml/launchpad/until-no-eintr
Reviewer Review Type Date Requested Status
Julian Edwards (community) Approve
Review via email: mp+33095@code.launchpad.net

Commit message

Add lp.services.osutils.until_no_eintr. Make builder use it.

Description of the change

Julian asked me to do this the other day, after the cherrypick had landed.

This branch adds a generic until_no_eintr helper, much like the ones in Bazaar and Twisted, but this time with a maximum retry limit. It then changes builder.py to use our until_no_eintr helper.

There are tests, too.

To post a comment you must log in.
Revision history for this message
Julian Edwards (julian-edwards) wrote :

Thanks for the change! Looks good, land with the suggestions we discussed in IRC.

Cheers.

<jml> a soyuz person might like to review this: https://code.edge.launchpad.net/~jml/launchpad/until-no-eintr/+merge/33095
<noodles> jml: sure (I'm ocr today anyway :) ).
<jml> noodles, cool, thanks.
<jml> it's a patch bigjools asked for :)
<bigjools> jml: fail on python2.5
<jml> bigjools, why so?
<bigjools> e.errno doesn't work
<bigjools> you need e[0]
-*- noodles leaves the review to bigjools and continues with other stuff :)
<bigjools> lol
<noodles> lunch-time!
<bigjools> jml: also, does IOError and OSError provide the same context?
<jml> bigjools, ahh, but only for socket.errro
<jml> bigjools, I don't understand the question.
<bigjools> "e" in your code
<bigjools> exception value
<jml> bigjools, an instance of either IOError or OSError has an 'errno' attribute
<jml> bigjools, regardless of Python version.
<bigjools> ok cool - but they are not returned in py 2.5 are they?
<bigjools> it returns socket.error
<jml> bigjools, they are raised by lots of things in Python 2.5
<jml> bigjools, just not the ones that happen to affect builder.py
<bigjools> ok
<jml> (e.g. you can get an IOError for an EINTR reading from a file object)
<bigjools> jml: so you should probably catch socket.error separately and use e[0] with the same comment in my original code
<jml> bigjools, will do.
<jml> bigjools, there are enough warts in this function that I'm glad it's being split out :)
<bigjools> quite :)
<bigjools> jml: is there a reason you picked "5" in the tests?
<jml> bigjools, it's less than 10 and more than 1.
<jml> bigjools, want me to give it an intent-revealing name?
<bigjools> so is 2 :)
<bigjools> yes please
<bigjools> at the test class level since you use it in most of them

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/buildmaster/model/builder.py'
2--- lib/lp/buildmaster/model/builder.py 2010-08-16 13:17:43 +0000
3+++ lib/lp/buildmaster/model/builder.py 2010-08-19 11:31:46 +0000
4@@ -12,7 +12,6 @@
5 'updateBuilderStatus',
6 ]
7
8-import errno
9 import httplib
10 import gzip
11 import logging
12@@ -53,6 +52,7 @@
13 from lp.registry.interfaces.person import validate_public_person
14 from lp.services.job.interfaces.job import JobStatus
15 from lp.services.job.model.job import Job
16+from lp.services.osutils import until_no_eintr
17 # XXX Michael Nelson 2010-01-13 bug=491330
18 # These dependencies on soyuz will be removed when getBuildRecords()
19 # is moved.
20@@ -196,43 +196,40 @@
21 (builder.name, slave_build_id, reason))
22
23
24+def _update_builder_status(builder, logger=None):
25+ """Really update the builder status."""
26+ try:
27+ builder.checkSlaveAlive()
28+ builder.rescueIfLost(logger)
29+ # Catch only known exceptions.
30+ # XXX cprov 2007-06-15 bug=120571: ValueError & TypeError catching is
31+ # disturbing in this context. We should spend sometime sanitizing the
32+ # exceptions raised in the Builder API since we already started the
33+ # main refactoring of this area.
34+ except (ValueError, TypeError, xmlrpclib.Fault,
35+ BuildDaemonError), reason:
36+ builder.failBuilder(str(reason))
37+ if logger:
38+ logger.warn(
39+ "%s (%s) marked as failed due to: %s",
40+ builder.name, builder.url, builder.failnotes, exc_info=True)
41+
42+
43 def updateBuilderStatus(builder, logger=None):
44 """See `IBuilder`."""
45 if logger:
46 logger.debug('Checking %s' % builder.name)
47
48 MAX_EINTR_RETRIES = 42 # pulling a number out of my a$$ here
49- eintr_retry_count = 0
50-
51- while True:
52- try:
53- builder.checkSlaveAlive()
54- builder.rescueIfLost(logger)
55- # Catch only known exceptions.
56- # XXX cprov 2007-06-15 bug=120571: ValueError & TypeError catching is
57- # disturbing in this context. We should spend sometime sanitizing the
58- # exceptions raised in the Builder API since we already started the
59- # main refactoring of this area.
60- except (ValueError, TypeError, xmlrpclib.Fault,
61- BuildDaemonError), reason:
62- builder.failBuilder(str(reason))
63- if logger:
64- logger.warn(
65- "%s (%s) marked as failed due to: %s",
66- builder.name, builder.url, builder.failnotes, exc_info=True)
67- except socket.error, reason:
68- # In Python 2.6 we can use IOError instead. It also has
69- # reason.errno but we might be using 2.5 here so use the
70- # index hack.
71- if reason[0] == errno.EINTR:
72- eintr_retry_count += 1
73- if eintr_retry_count != MAX_EINTR_RETRIES:
74- # It was an EINTR. Just retry.
75- continue
76- error_message = str(reason)
77- builder.handleTimeout(logger, error_message)
78-
79- return
80+ try:
81+ return until_no_eintr(
82+ MAX_EINTR_RETRIES, _update_builder_status, builder, logger=logger)
83+ except socket.error, reason:
84+ # In Python 2.6 we can use IOError instead. It also has
85+ # reason.errno but we might be using 2.5 here so use the
86+ # index hack.
87+ error_message = str(reason)
88+ builder.handleTimeout(logger, error_message)
89
90
91 class Builder(SQLBase):
92
93=== modified file 'lib/lp/services/osutils.py'
94--- lib/lp/services/osutils.py 2010-08-06 18:55:48 +0000
95+++ lib/lp/services/osutils.py 2010-08-19 11:31:46 +0000
96@@ -1,4 +1,4 @@
97-# Copyright 2009 Canonical Ltd. This software is licensed under the
98+# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
99 # GNU Affero General Public License version 3 (see the file LICENSE).
100
101 """Utilities for doing the sort of thing the os module does."""
102@@ -10,10 +10,13 @@
103 'kill_by_pidfile',
104 'remove_if_exists',
105 'two_stage_kill',
106+ 'until_no_eintr',
107 ]
108
109 from contextlib import contextmanager
110+import errno
111 import os.path
112+import socket
113 import shutil
114
115
116@@ -58,3 +61,32 @@
117 yield
118 finally:
119 set_environ(old_values)
120+
121+
122+def until_no_eintr(retries, function, *args, **kwargs):
123+ """Run 'function' until it doesn't raise EINTR errors.
124+
125+ :param retries: The maximum number of times to try running 'function'.
126+ :param function: The function to run.
127+ :param *args: Arguments passed to the function.
128+ :param **kwargs: Keyword arguments passed to the function.
129+ :return: The return value of 'function'.
130+ """
131+ if not retries:
132+ return
133+ for i in range(retries):
134+ try:
135+ return function(*args, **kwargs)
136+ except (IOError, OSError), e:
137+ if e.errno == errno.EINTR:
138+ continue
139+ raise
140+ except socket.error, e:
141+ # In Python 2.6 we can use IOError instead. It also has
142+ # reason.errno but we might be using 2.5 here so use the
143+ # index hack.
144+ if e[0] == errno.EINTR:
145+ continue
146+ raise
147+ else:
148+ raise
149
150=== modified file 'lib/lp/services/tests/test_osutils.py'
151--- lib/lp/services/tests/test_osutils.py 2009-07-17 02:25:09 +0000
152+++ lib/lp/services/tests/test_osutils.py 2010-08-19 11:31:46 +0000
153@@ -5,11 +5,16 @@
154
155 __metaclass__ = type
156
157+import errno
158 import os
159+import socket
160 import tempfile
161 import unittest
162
163-from lp.services.osutils import remove_tree
164+from lp.services.osutils import (
165+ remove_tree,
166+ until_no_eintr,
167+ )
168 from lp.testing import TestCase
169
170
171@@ -41,5 +46,102 @@
172 self.assertRaises(OSError, remove_tree, filename)
173
174
175+class TestUntilNoEINTR(TestCase):
176+ """Tests for until_no_eintr."""
177+
178+ # The maximum number of retries used in our tests.
179+ MAX_RETRIES = 10
180+
181+ # A number of retries less than the maximum number used in tests.
182+ SOME_RETRIES = MAX_RETRIES / 2
183+
184+ def test_no_calls(self):
185+ # If the user has, bizarrely, asked for 0 attempts, then never try to
186+ # call the function.
187+ calls = []
188+ until_no_eintr(0, calls.append, None)
189+ self.assertEqual([], calls)
190+
191+ def test_function_doesnt_raise(self):
192+ # If the function doesn't raise, call it only once.
193+ calls = []
194+ until_no_eintr(self.MAX_RETRIES, calls.append, None)
195+ self.assertEqual(1, len(calls))
196+
197+ def test_returns_function_return(self):
198+ # If the function doesn't raise, return its value.
199+ ret = until_no_eintr(1, lambda: 42)
200+ self.assertEqual(42, ret)
201+
202+ def test_raises_exception(self):
203+ # If the function raises an exception that's not EINTR, then re-raise
204+ # it.
205+ self.assertRaises(ZeroDivisionError, until_no_eintr, 1, lambda: 1/0)
206+
207+ def test_retries_on_ioerror_eintr(self):
208+ # Retry the function as long as it keeps raising IOError(EINTR).
209+ calls = []
210+ def function():
211+ calls.append(None)
212+ if len(calls) < self.SOME_RETRIES:
213+ raise IOError(errno.EINTR, os.strerror(errno.EINTR))
214+ return 'orange'
215+ ret = until_no_eintr(self.MAX_RETRIES, function)
216+ self.assertEqual(self.SOME_RETRIES, len(calls))
217+ self.assertEqual('orange', ret)
218+
219+ def test_retries_on_oserror_eintr(self):
220+ # Retry the function as long as it keeps raising OSError(EINTR).
221+ calls = []
222+ def function():
223+ calls.append(None)
224+ if len(calls) < self.SOME_RETRIES:
225+ raise OSError(errno.EINTR, os.strerror(errno.EINTR))
226+ return 'orange'
227+ ret = until_no_eintr(self.MAX_RETRIES, function)
228+ self.assertEqual(self.SOME_RETRIES, len(calls))
229+ self.assertEqual('orange', ret)
230+
231+ def test_retries_on_socket_error_eintr(self):
232+ # Retry the function as long as it keeps raising socket.error(EINTR).
233+ # This test is redundant on Python 2.6, since socket.error is an
234+ # IOError there.
235+ calls = []
236+ def function():
237+ calls.append(None)
238+ if len(calls) < self.SOME_RETRIES:
239+ raise socket.error(errno.EINTR, os.strerror(errno.EINTR))
240+ return 'orange'
241+ ret = until_no_eintr(self.MAX_RETRIES, function)
242+ self.assertEqual(self.SOME_RETRIES, len(calls))
243+ self.assertEqual('orange', ret)
244+
245+ def test_raises_other_error_without_retry(self):
246+ # Any other kind of IOError (or OSError or socket.error) is re-raised
247+ # with a retry attempt.
248+ calls = []
249+ def function():
250+ calls.append(None)
251+ if len(calls) < self.SOME_RETRIES:
252+ raise IOError(errno.ENOENT, os.strerror(errno.ENOENT))
253+ return 'orange'
254+ error = self.assertRaises(
255+ IOError, until_no_eintr, self.MAX_RETRIES, function)
256+ self.assertEqual(errno.ENOENT, error.errno)
257+ self.assertEqual(1, len(calls))
258+
259+ def test_never_exceeds_retries(self):
260+ # If the function keeps on raising EINTR, then stop running it after
261+ # the given number of retries, and just re-raise the error.
262+ calls = []
263+ def function():
264+ calls.append(None)
265+ raise IOError(errno.EINTR, os.strerror(errno.EINTR))
266+ error = self.assertRaises(
267+ IOError, until_no_eintr, self.MAX_RETRIES, function)
268+ self.assertEqual(errno.EINTR, error.errno)
269+ self.assertEqual(self.MAX_RETRIES, len(calls))
270+
271+
272 def test_suite():
273 return unittest.TestLoader().loadTestsFromName(__name__)