launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/unreliable-session-service into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #901844 in Launchpad itself: "unreliablesession service causing oops during after-request processing"
  https://bugs.launchpad.net/launchpad/+bug/901844

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/unreliable-session-service/+merge/129981

Summary
=======
Unreliable session can break on disconnect because of a socket error. But
truthfully, the work of the disconnect function is still done, so we shouldn't
error out on socket.error.

Preimp
======
Spoke with Curtis Hovey.

Implementation
==============
An except block for socket.error has been added to the disconnect method,
which suppresses the error.

Tests have been added.

Tests
=====
bin/test --vvct test_rabbit

QA
==
I believe this qa-untestable.

LoC
===
I have over 400 lines of LoC credit.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/services/messaging/rabbit.py
  lib/lp/services/messaging/tests/test_rabbit.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/unreliable-session-service/+merge/129981
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/unreliable-session-service into lp:launchpad.

=== modified file 'lib/lp/services/messaging/rabbit.py'
--- lib/lp/services/messaging/rabbit.py	2012-06-29 08:40:05 +0000
+++ lib/lp/services/messaging/rabbit.py	2012-10-16 20:42:26 +0000
@@ -15,6 +15,7 @@
 from functools import partial
 import json
 import sys
+import socket
 import threading
 import time
 
@@ -117,6 +118,9 @@
         if self._connection is not None:
             try:
                 self._connection.close()
+            except socket.error:
+                # Socket error is fine; the connection is still closed.
+                pass
             finally:
                 self._connection = None
 

=== modified file 'lib/lp/services/messaging/tests/test_rabbit.py'
--- lib/lp/services/messaging/tests/test_rabbit.py	2012-04-24 04:23:52 +0000
+++ lib/lp/services/messaging/tests/test_rabbit.py	2012-10-16 20:42:26 +0000
@@ -7,6 +7,7 @@
 
 from functools import partial
 from itertools import count
+import socket
 import thread
 
 from testtools.testcase import ExpectedException
@@ -123,6 +124,17 @@
         session.disconnect()
         self.assertFalse(session.is_connected)
 
+    def test_disconnect_with_error(self):
+        session = self.session_factory()
+        session.connect()
+        old_close = session._connection.close
+        def new_close(*args, **kwargs):
+            old_close(*args, **kwargs)
+            raise socket.error
+        session._connection.close = new_close
+        session.disconnect()
+        self.assertFalse(session.is_connected)
+
     def test_is_connected(self):
         # is_connected is False once a connection has been closed.
         session = self.session_factory()