launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/python-oops-amqp/0.0.3 into lp:python-oops-amqp.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #882346 in python-oops-amqp: "IOError exceptions are not caught"
  https://bugs.launchpad.net/python-oops-amqp/+bug/882346

For more details, see:
https://code.launchpad.net/~lifeless/python-oops-amqp/0.0.3/+merge/80524

Handle IOError raised from within amqplib. This is sadly broad so I've narrowed it by checking the args. However, as its near-impossible (Read I haven't managed via automated means) to trigger this, I can't test it. Mocking things out would give false coverage: we're at primary risk of skew if amqplib decides to 'improve' its behaviour here. So - no tests :(. Other than that, should be straight forward and the existing socket.error tests should cover most situations.
-- 
https://code.launchpad.net/~lifeless/python-oops-amqp/0.0.3/+merge/80524
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/python-oops-amqp/0.0.3 into lp:python-oops-amqp.

=== modified file 'NEWS'
--- NEWS	2011-10-18 02:26:11 +0000
+++ NEWS	2011-10-27 02:00:28 +0000
@@ -6,6 +6,13 @@
 NEXT
 ----
 
+0.0.3
+-----
+
+* ampqlib raised IOError errors are now caught as well. This is plumbing
+  dependent and thus may need updating when working with different amqplib
+  versions. (Robert Collins, #882346)
+
 0.0.2
 -----
 

=== modified file 'oops_amqp/__init__.py'
--- oops_amqp/__init__.py	2011-10-17 22:00:57 +0000
+++ oops_amqp/__init__.py	2011-10-27 02:00:28 +0000
@@ -97,7 +97,7 @@
 # established at this point, and setup.py will use a version of next-$(revno).
 # If the releaselevel is 'final', then the tarball will be major.minor.micro.
 # Otherwise it is major.minor.micro~$(revno).
-__version__ = (0, 0, 2, 'beta', 0)
+__version__ = (0, 0, 3, 'beta', 0)
 
 __all__ = [
     'Publisher',

=== modified file 'oops_amqp/publisher.py'
--- oops_amqp/publisher.py	2011-10-18 02:26:11 +0000
+++ oops_amqp/publisher.py	2011-10-27 02:00:28 +0000
@@ -25,6 +25,8 @@
 from amqplib import client_0_8 as amqp
 from bson import dumps
 
+from utils import is_amqplib_connection_error
+
 __all__ = [
     'Publisher',
     ]
@@ -61,9 +63,12 @@
         if getattr(self.channels, 'channel', None) is None:
             try:
                 self.channels.channel = self.connection_factory().channel()
-            except socket.error:
-                # Could not connect
-                return None
+            except (socket.error, IOError), e:
+                if is_amqplib_connection_error(e):
+                    # Could not connect
+                    return None
+                # Unknown error mode : don't hide it.
+                raise
         return self.channels.channel
 
     def __call__(self, report):
@@ -86,7 +91,11 @@
         try:
             channel.basic_publish(
                 message, self.exchange_name, routing_key=self.routing_key)
-        except socket.error:
+        except (socket.error, IOError), e:
             self.channels.channel = None
-            return None
+            if is_amqplib_connection_error(e):
+                # Could not connect / interrupted connection
+                return None
+            # Unknown error mode : don't hide it.
+            raise
         return report['id']

=== modified file 'oops_amqp/receiver.py'
--- oops_amqp/receiver.py	2011-10-18 03:19:09 +0000
+++ oops_amqp/receiver.py	2011-10-27 02:00:28 +0000
@@ -23,7 +23,10 @@
 
 import bson
 
-from utils import close_ignoring_EPIPE
+from utils import (
+    close_ignoring_EPIPE,
+    is_amqplib_connection_error,
+    )
 
 __all__ = [
     'Receiver',
@@ -81,7 +84,10 @@
                (not self.went_bad or time.time() < self.went_bad + 120)):
             try:
                 self._run_forever()
-            except socket.error:
+            except (socket.error, IOError), e:
+                if not is_amqplib_connection_error(e):
+                    # Something unknown went wrong.
+                    raise
                 if not self.went_bad:
                     self.went_bad = time.time()
                 # Don't probe immediately, give the network/process time to

=== modified file 'oops_amqp/utils.py'
--- oops_amqp/utils.py	2011-10-17 23:26:09 +0000
+++ oops_amqp/utils.py	2011-10-27 02:00:28 +0000
@@ -21,6 +21,8 @@
 
 __all__ = [
     'close_ignoring_EPIPE',
+    'is_amqplib_ioerror',
+    'is_amqplib_connection_error',
     ]
 
 
@@ -30,3 +32,14 @@
     except socket.error, e:
         if e.errno != errno.EPIPE:
             raise
+
+
+def is_amqplib_ioerror(e):
+    """Returns True if e is an amqplib internal exception."""
+    # Raised by amqplib rather than socket.error on ssl issues and short reads.
+    return type(e) is IOError and e.args == ('Socket error',)
+
+
+def is_amqplib_connection_error(e):
+    """Return True if e was (probably) raised due to a connection issue."""
+    return isinstance(e, socket.error) or is_amqplib_ioerror(e)

=== modified file 'setup.py'
--- setup.py	2011-10-17 22:00:57 +0000
+++ setup.py	2011-10-27 02:00:28 +0000
@@ -1,4 +1,7 @@
 #!/usr/bin/env python
+
+0.0.3
+-----
 #
 # Copyright (c) 2011, Canonical Ltd
 #
@@ -23,7 +26,7 @@
         os.path.join(os.path.dirname(__file__), 'README'), 'rb').read()
 
 setup(name="oops_amqp",
-      version="0.0.2",
+      version="0.0.3",
       description=\
               "OOPS AMQP transport.",
       long_description=description,