[Zodb-checkins] CVS: ZEO/ZEO/zrpc - client.py:1.1.2.10

[Jeremy Hylton]

Update of /cvs-repository/ZEO/ZEO/zrpc
In directory cvs.zope.org:/tmp/cvs-serv10307

Modified Files:
      Tag: Standby-branch
	client.py 
Log Message:
Fix several corner cases in the ConnectThread logic.

Catch and log errors from more socket calls.  It's possible for even
connect_ex() to raise an exception, e.g. "host not found."

Make sure that every path through connect() method does one of the
following: 

    - Returns without doing anything if connect_ex() returns
      EINPROGRESS or similar.

    - Raises a Connected() exception if the zrpc connection is
      established.

    - Closes the socket, removes it from self.sockets, and logs an
      error in all other cases.

Address Tim's XXX comments.


=== ZEO/ZEO/zrpc/client.py 1.1.2.9 => 1.1.2.10 ===
                     log("attempt connection to %s" % repr(addr),
                         level=zLOG.DEBUG)
-                s = socket.socket(domain, socket.SOCK_STREAM)
+                try:
+                    s = socket.socket(domain, socket.SOCK_STREAM)
+                except socket.error, err:
+                    log("Failed to create socket with domain=%s: %s" % (
+                        domain, err), level=zLOG.ERROR)
+                    continue
                 s.setblocking(0)
                 self.sockets[s] = addr
                 # connect() raises Connected iff it succeeds
@@ -262,33 +267,42 @@
         select() loop in the caller and an exception is a principled
         way to do the abort.
 
-        If the socket sonnects and the initial ZEO setup fails or the
-        connect_ex() returns an error, we close the socket and ignore it.
-        XXX If test_connection() fails (I assume that's what's meant by
-        XXX "the initial ZEO setup fails", the code below doesn't do anything
-        XXX with the socket.  So the comment or the code is wrong.
-        When the socket is closed, it is removed from self.sockets.
+        If the socket sonnects and the initial ZEO setup
+        (notifyConnected()) fails or the connect_ex() returns an
+        error, we close the socket, remove it from self.sockets, and
+        proceed with the other sockets.
 
         If connect_ex() returns EINPROGRESS, we need to try again later.
         """
         addr = self.sockets[s]
-        e = s.connect_ex(addr)
-        if e in _CONNECT_IN_PROGRESS:
-            pass
-        elif e in _CONNECT_OK:
-            c = self.test_connection(s, addr)
-            # XXX we should't log that we got connected if c==0, should we?
-            log("connected to %s" % repr(addr), level=zLOG.DEBUG)
-            if c:
-                raise Connected(s)
+        try:
+            e = s.connect_ex(addr)
+        except socket.error, msg:
+            log("failed to connect to %s: %s" % (addr, msg),
+                level=zLOG.ERROR)
         else:
-            if __debug__:
+            if e in _CONNECT_IN_PROGRESS:
+                return
+            elif e in _CONNECT_OK:
+                c = self.test_connection(s, addr)
+                if c:
+                    log("connected to %s" % repr(addr), level=zLOG.DEBUG)
+                    raise Connected(s)
+            else:
                 log("error connecting to %s: %s" % (addr, errno.errorcode[e]),
                     level=zLOG.DEBUG)
-            s.close()
-            del self.sockets[s]
+        # Any execution that doesn't raise Connected() or return
+        # because of CONNECT_IN_PROGRESS is an error.  Make sure the
+        # socket is closed and remove it from the dict of pending
+        # sockets.
+        s.close()
+        del self.sockets[s]
 
     def test_connection(self, s, addr):
+        # Establish a connection at the zrpc level and call the
+        # client's notifyConnected(), giving the zrpc application a
+        # chance to do app-level check of whether the connection is
+        # okay.
         c = ManagedConnection(s, addr, self.client, self.mgr)
         try:
             self.client.notifyConnected(c)
@@ -296,9 +310,8 @@
             log("error connecting to server: %s" % str(addr),
                 level=zLOG.ERROR, error=sys.exc_info())
             c.close()
-            # XXX what's the state of s at this point?  If s got closed,
-            # XXX self.sockets contains a closed socket, and the
-            # XXX next attempt_connects() select() will blow up.
+            # Closing the ZRPC connection will eventually close the
+            # socket, somewhere in asyncore.
             return 0
         self.mgr.connect_done(c)
         return 1