Merge lp:~bac/python-memcached/bug-974632 into lp:python-memcached

[Brad Crittenden]

Restore the previous behavior of readline() so that it returns None if the socket is closed. A new internal method _readline() is introduced to provide the uncaught _ConnectionDeadError exception for the places that wish to catch it and do a retry.

[Sean Reifschneider (jafo)]

I think this misses some cases that the current could would also raise an exception in. This is a good start, and helped me understand the problem, thanks for the patch, but I've made some other changes which include restoring the readline() back to not raising an exception in most cases, and the _unsafe_*() functions calling functions which explicitly request that readline() raise the exception.

I'm going to commit a change shortly, would you mind reviewing it and seeing if it fixes the issue?

Thanks,
Sean

review: Disapprove

[Brad Crittenden (bac)]

Hi Sean, thanks for looking at this code.

Your comment confuses me a little because the approach you describe as your preferred solution seems to be what I've implemented, that is the readline() method is restored to never throw an exception while the unsafe do call a special method that can raise the exception. I called this new method _readline(), which could've been named better.

Perhaps there are call sites that use _expectvalue that I missed?

Anyway, I'll be happy to review the patch you produce.

[Sean Reifschneider (jafo)]

On 04/11/2012 04:37 AM, Brad Crittenden wrote:
> Your comment confuses me a little because the approach you describe

I guess the simplest explanation I can give is that my patches changes the
semantics back to how they were before Tarek's patch -- exceptions aren't
raised by that code. Only in the cases where the higher-level code
explicitly requests the exceptions are they generated.

I don't recall exactly what I saw that I thought might still leak
exceptions, but it just "felt right" this way, basically reverting to the
old behavior, regarding exception generation.

Thoughts?

Thanks,
Sean

Unmerged revisions

57. By Brad Crittenden on 2012-04-06

Provide a wrapper to handled _ConnectionDeadErrors so that readline() behaves as before.

=== modified file 'memcache.py'
--- memcache.py	2011-11-29 19:34:32 +0000
+++ memcache.py	2012-04-06 19:18:22 +0000
@@ -182,7 +182,7 @@
         @param cache_cas: (default False) If true, cas operations will be
         cached.  WARNING: This cache is not expired internally, if you have
         a long-running process you will need to expire it manually via
-        "client.reset_cas(), or the cache can grow unlimited.
+        client.reset_cas(), or the cache can grow unlimited.
         @param server_max_key_length: (default SERVER_MAX_KEY_LENGTH)
         Data that is larger than this will not be sent to the server.
         @param server_max_value_length: (default SERVER_MAX_VALUE_LENGTH)
@@ -960,7 +960,7 @@
 
     def _expectvalue(self, server, line=None):
         if not line:
-            line = server.readline()
+            line = server._readline()
 
         if line and line[:5] == 'VALUE':
             resp, rkey, flags, len = line.split()
@@ -1064,8 +1064,8 @@
         else:
             self.family = socket.AF_INET
             self.ip = hostData['host']
-            self.port = int(hostData.get('port', 11211))
-            self.address = ( self.ip, self.port )
+            self.port = int(hostData.get('port') or 11211)
+            self.address = (self.ip, self.port)
 
         self.deaduntil = 0
         self.socket = None
@@ -1130,7 +1130,11 @@
         """ cmds already has trailing \r\n's applied """
         self.socket.sendall(cmds)
 
-    def readline(self):
+    def _readline(self):
+        """Read from the socket.
+
+        If the socket is closed (Raises _ConnectionDeadError).
+        """
         buf = self.buffer
         recv = self.socket.recv
         while True:
@@ -1147,6 +1151,13 @@
         self.buffer = buf[index+2:]
         return buf[:index]
 
+    def readline(self):
+        """Read a line.  If the connection is dead, return None."""
+        try:
+            return self._readline()
+        except _ConnectionDeadError:
+            return None
+
     def expect(self, text):
         line = self.readline()
         if line != text: