python-oops-amqp

Merge lp:~lifeless/python-oops-amqp/0.0.3 into lp:python-oops-amqp

  1. 0.0.3
  2. Merge into trunk
Proposed by Robert Collins
Status: Merged
Merged at revision: 4
Proposed branch: lp:~lifeless/python-oops-amqp/0.0.3
Merge into: lp:python-oops-amqp
Diff against target: 150 lines (+47/-9)
6 files modified
NEWS (+7/-0)
oops_amqp/__init__.py (+1/-1)
oops_amqp/publisher.py (+14/-5)
oops_amqp/receiver.py (+8/-2)
oops_amqp/utils.py (+13/-0)
setup.py (+4/-1)
To merge this branch: bzr merge lp:~lifeless/python-oops-amqp/0.0.3
Reviewer Review Type Date Requested Status
Steve Kowalik (community) code Approve
Review via email: mp+80524@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Steve Kowalik (stevenk) wrote :

You're right, this does look a little odd, and the lack of tests also makes me sad.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2011-10-18 02:26:11 +0000
3+++ NEWS 2011-10-27 02:00:28 +0000
4@@ -6,6 +6,13 @@
5 NEXT
6 ----
7
8+0.0.3
9+-----
10+
11+* ampqlib raised IOError errors are now caught as well. This is plumbing
12+ dependent and thus may need updating when working with different amqplib
13+ versions. (Robert Collins, #882346)
14+
15 0.0.2
16 -----
17
18
19=== modified file 'oops_amqp/__init__.py'
20--- oops_amqp/__init__.py 2011-10-17 22:00:57 +0000
21+++ oops_amqp/__init__.py 2011-10-27 02:00:28 +0000
22@@ -97,7 +97,7 @@
23 # established at this point, and setup.py will use a version of next-$(revno).
24 # If the releaselevel is 'final', then the tarball will be major.minor.micro.
25 # Otherwise it is major.minor.micro~$(revno).
26-__version__ = (0, 0, 2, 'beta', 0)
27+__version__ = (0, 0, 3, 'beta', 0)
28
29 __all__ = [
30 'Publisher',
31
32=== modified file 'oops_amqp/publisher.py'
33--- oops_amqp/publisher.py 2011-10-18 02:26:11 +0000
34+++ oops_amqp/publisher.py 2011-10-27 02:00:28 +0000
35@@ -25,6 +25,8 @@
36 from amqplib import client_0_8 as amqp
37 from bson import dumps
38
39+from utils import is_amqplib_connection_error
40+
41 __all__ = [
42 'Publisher',
43 ]
44@@ -61,9 +63,12 @@
45 if getattr(self.channels, 'channel', None) is None:
46 try:
47 self.channels.channel = self.connection_factory().channel()
48- except socket.error:
49- # Could not connect
50- return None
51+ except (socket.error, IOError), e:
52+ if is_amqplib_connection_error(e):
53+ # Could not connect
54+ return None
55+ # Unknown error mode : don't hide it.
56+ raise
57 return self.channels.channel
58
59 def __call__(self, report):
60@@ -86,7 +91,11 @@
61 try:
62 channel.basic_publish(
63 message, self.exchange_name, routing_key=self.routing_key)
64- except socket.error:
65+ except (socket.error, IOError), e:
66 self.channels.channel = None
67- return None
68+ if is_amqplib_connection_error(e):
69+ # Could not connect / interrupted connection
70+ return None
71+ # Unknown error mode : don't hide it.
72+ raise
73 return report['id']
74
75=== modified file 'oops_amqp/receiver.py'
76--- oops_amqp/receiver.py 2011-10-18 03:19:09 +0000
77+++ oops_amqp/receiver.py 2011-10-27 02:00:28 +0000
78@@ -23,7 +23,10 @@
79
80 import bson
81
82-from utils import close_ignoring_EPIPE
83+from utils import (
84+ close_ignoring_EPIPE,
85+ is_amqplib_connection_error,
86+ )
87
88 __all__ = [
89 'Receiver',
90@@ -81,7 +84,10 @@
91 (not self.went_bad or time.time() < self.went_bad + 120)):
92 try:
93 self._run_forever()
94- except socket.error:
95+ except (socket.error, IOError), e:
96+ if not is_amqplib_connection_error(e):
97+ # Something unknown went wrong.
98+ raise
99 if not self.went_bad:
100 self.went_bad = time.time()
101 # Don't probe immediately, give the network/process time to
102
103=== modified file 'oops_amqp/utils.py'
104--- oops_amqp/utils.py 2011-10-17 23:26:09 +0000
105+++ oops_amqp/utils.py 2011-10-27 02:00:28 +0000
106@@ -21,6 +21,8 @@
107
108 __all__ = [
109 'close_ignoring_EPIPE',
110+ 'is_amqplib_ioerror',
111+ 'is_amqplib_connection_error',
112 ]
113
114
115@@ -30,3 +32,14 @@
116 except socket.error, e:
117 if e.errno != errno.EPIPE:
118 raise
119+
120+
121+def is_amqplib_ioerror(e):
122+ """Returns True if e is an amqplib internal exception."""
123+ # Raised by amqplib rather than socket.error on ssl issues and short reads.
124+ return type(e) is IOError and e.args == ('Socket error',)
125+
126+
127+def is_amqplib_connection_error(e):
128+ """Return True if e was (probably) raised due to a connection issue."""
129+ return isinstance(e, socket.error) or is_amqplib_ioerror(e)
130
131=== modified file 'setup.py'
132--- setup.py 2011-10-17 22:00:57 +0000
133+++ setup.py 2011-10-27 02:00:28 +0000
134@@ -1,4 +1,7 @@
135 #!/usr/bin/env python
136+
137+0.0.3
138+-----
139 #
140 # Copyright (c) 2011, Canonical Ltd
141 #
142@@ -23,7 +26,7 @@
143 os.path.join(os.path.dirname(__file__), 'README'), 'rb').read()
144
145 setup(name="oops_amqp",
146- version="0.0.2",
147+ version="0.0.3",
148 description=\
149 "OOPS AMQP transport.",
150 long_description=description,

Subscribers

People subscribed via source and target branches

to all changes: