launchpad-reviewers team mailing list archive

[Jonathan Lange <jml@xxxxxxxxxxxxx>]

Jonathan Lange has proposed merging lp:~jml/launchpad/until-no-eintr into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


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.

-- 
https://code.launchpad.net/~jml/launchpad/until-no-eintr/+merge/33095
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jml/launchpad/until-no-eintr into lp:launchpad/devel.

=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2010-08-16 13:17:43 +0000
+++ lib/lp/buildmaster/model/builder.py	2010-08-19 11:05:58 +0000
@@ -12,7 +12,6 @@
     'updateBuilderStatus',
     ]
 
-import errno
 import httplib
 import gzip
 import logging
@@ -53,6 +52,7 @@
 from lp.registry.interfaces.person import validate_public_person
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
+from lp.services.osutils import until_no_eintr
 # XXX Michael Nelson 2010-01-13 bug=491330
 # These dependencies on soyuz will be removed when getBuildRecords()
 # is moved.
@@ -196,43 +196,40 @@
                 (builder.name, slave_build_id, reason))
 
 
+def _update_builder_status(builder, logger=None):
+    """Really update the builder status."""
+    try:
+        builder.checkSlaveAlive()
+        builder.rescueIfLost(logger)
+    # Catch only known exceptions.
+    # XXX cprov 2007-06-15 bug=120571: ValueError & TypeError catching is
+    # disturbing in this context. We should spend sometime sanitizing the
+    # exceptions raised in the Builder API since we already started the
+    # main refactoring of this area.
+    except (ValueError, TypeError, xmlrpclib.Fault,
+            BuildDaemonError), reason:
+        builder.failBuilder(str(reason))
+        if logger:
+            logger.warn(
+                "%s (%s) marked as failed due to: %s",
+                builder.name, builder.url, builder.failnotes, exc_info=True)
+
+
 def updateBuilderStatus(builder, logger=None):
     """See `IBuilder`."""
     if logger:
         logger.debug('Checking %s' % builder.name)
 
     MAX_EINTR_RETRIES = 42 # pulling a number out of my a$$ here
-    eintr_retry_count = 0
-
-    while True:
-        try:
-            builder.checkSlaveAlive()
-            builder.rescueIfLost(logger)
-        # Catch only known exceptions.
-        # XXX cprov 2007-06-15 bug=120571: ValueError & TypeError catching is
-        # disturbing in this context. We should spend sometime sanitizing the
-        # exceptions raised in the Builder API since we already started the
-        # main refactoring of this area.
-        except (ValueError, TypeError, xmlrpclib.Fault,
-                BuildDaemonError), reason:
-            builder.failBuilder(str(reason))
-            if logger:
-                logger.warn(
-                    "%s (%s) marked as failed due to: %s",
-                    builder.name, builder.url, builder.failnotes, exc_info=True)
-        except socket.error, reason:
-            # In Python 2.6 we can use IOError instead.  It also has
-            # reason.errno but we might be using 2.5 here so use the
-            # index hack.
-            if reason[0] == errno.EINTR:
-                eintr_retry_count += 1
-                if eintr_retry_count != MAX_EINTR_RETRIES:
-                    # It was an EINTR. Just retry.
-                    continue
-            error_message = str(reason)
-            builder.handleTimeout(logger, error_message)
-
-        return
+    try:
+        return until_no_eintr(
+            MAX_EINTR_RETRIES, _update_builder_status, builder, logger=logger)
+    except socket.error, reason:
+        # In Python 2.6 we can use IOError instead.  It also has
+        # reason.errno but we might be using 2.5 here so use the
+        # index hack.
+        error_message = str(reason)
+        builder.handleTimeout(logger, error_message)
 
 
 class Builder(SQLBase):

=== modified file 'lib/lp/services/osutils.py'
--- lib/lp/services/osutils.py	2010-08-06 18:55:48 +0000
+++ lib/lp/services/osutils.py	2010-08-19 11:05:58 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Utilities for doing the sort of thing the os module does."""
@@ -10,10 +10,13 @@
     'kill_by_pidfile',
     'remove_if_exists',
     'two_stage_kill',
+    'until_no_eintr',
     ]
 
 from contextlib import contextmanager
+import errno
 import os.path
+import socket
 import shutil
 
 
@@ -58,3 +61,25 @@
         yield
     finally:
         set_environ(old_values)
+
+
+def until_no_eintr(retries, function, *args, **kwargs):
+    """Run 'function' until it doesn't raise EINTR errors.
+
+    :param retries: The maximum number of times to try running 'function'.
+    :param function: The function to run.
+    :param *args: Arguments passed to the function.
+    :param **kwargs: Keyword arguments passed to the function.
+    :return: The return value of 'function'.
+    """
+    if not retries:
+        return
+    for i in range(retries):
+        try:
+            return function(*args, **kwargs)
+        except (IOError, OSError, socket.error), e:
+            if e.errno == errno.EINTR:
+                continue
+            raise
+    else:
+        raise

=== modified file 'lib/lp/services/tests/test_osutils.py'
--- lib/lp/services/tests/test_osutils.py	2009-07-17 02:25:09 +0000
+++ lib/lp/services/tests/test_osutils.py	2010-08-19 11:05:58 +0000
@@ -5,11 +5,16 @@
 
 __metaclass__ = type
 
+import errno
 import os
+import socket
 import tempfile
 import unittest
 
-from lp.services.osutils import remove_tree
+from lp.services.osutils import (
+    remove_tree,
+    until_no_eintr,
+    )
 from lp.testing import TestCase
 
 
@@ -41,5 +46,94 @@
         self.assertRaises(OSError, remove_tree, filename)
 
 
+class TestUntilNoEINTR(TestCase):
+    """Tests for until_no_eintr."""
+
+    def test_no_calls(self):
+        # If the user has, bizarrely, asked for 0 attempts, then never try to
+        # call the function.
+        calls = []
+        until_no_eintr(0, calls.append, None)
+        self.assertEqual([], calls)
+
+    def test_function_doesnt_raise(self):
+        # If the function doesn't raise, call it only once.
+        calls = []
+        until_no_eintr(10, calls.append, None)
+        self.assertEqual(1, len(calls))
+
+    def test_returns_function_return(self):
+        # If the function doesn't raise, return its value.
+        ret = until_no_eintr(1, lambda: 42)
+        self.assertEqual(42, ret)
+
+    def test_raises_exception(self):
+        # If the function raises an exception that's not EINTR, then re-raise
+        # it.
+        self.assertRaises(ZeroDivisionError, until_no_eintr, 1, lambda: 1/0)
+
+    def test_retries_on_ioerror_eintr(self):
+        # Retry the function as long as it keeps raising IOError(EINTR).
+        calls = []
+        def function():
+            calls.append(None)
+            if len(calls) < 5:
+                raise IOError(errno.EINTR, os.strerror(errno.EINTR))
+            return 'orange'
+        ret = until_no_eintr(10, function)
+        self.assertEqual(5, len(calls))
+        self.assertEqual('orange', ret)
+
+    def test_retries_on_oserror_eintr(self):
+        # Retry the function as long as it keeps raising OSError(EINTR).
+        calls = []
+        def function():
+            calls.append(None)
+            if len(calls) < 5:
+                raise OSError(errno.EINTR, os.strerror(errno.EINTR))
+            return 'orange'
+        ret = until_no_eintr(10, function)
+        self.assertEqual(5, len(calls))
+        self.assertEqual('orange', ret)
+
+    def test_retries_on_socket_error_eintr(self):
+        # Retry the function as long as it keeps raising socket.error(EINTR).
+        # This test is redundant on Python 2.6, since socket.error is an
+        # IOError there.
+        calls = []
+        def function():
+            calls.append(None)
+            if len(calls) < 5:
+                raise socket.error(errno.EINTR, os.strerror(errno.EINTR))
+            return 'orange'
+        ret = until_no_eintr(10, function)
+        self.assertEqual(5, len(calls))
+        self.assertEqual('orange', ret)
+
+    def test_raises_other_error_without_retry(self):
+        # Any other kind of IOError (or OSError or socket.error) is re-raised
+        # with a retry attempt.
+        calls = []
+        def function():
+            calls.append(None)
+            if len(calls) < 5:
+                raise IOError(errno.ENOENT, os.strerror(errno.ENOENT))
+            return 'orange'
+        error = self.assertRaises(IOError, until_no_eintr, 10, function)
+        self.assertEqual(errno.ENOENT, error.errno)
+        self.assertEqual(1, len(calls))
+
+    def test_never_exceeds_retries(self):
+        # If the function keeps on raising EINTR, then stop running it after
+        # the given number of retries, and just re-raise the error.
+        calls = []
+        def function():
+            calls.append(None)
+            raise IOError(errno.EINTR, os.strerror(errno.EINTR))
+        error = self.assertRaises(IOError, until_no_eintr, 10, function)
+        self.assertEqual(errno.EINTR, error.errno)
+        self.assertEqual(10, len(calls))
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)