launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging ~wgrant/turnip:half-close-rework into turnip:master.

Commit message:
Limit half-closed connection handling to clients that are done writing.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/turnip/+git/turnip/+merge/288617

Limit half-closed connection handling to clients that are done writing.

Previously we somewhat dodgily implemented both types of half-closure in
both directions, but half-closure is only actually needed to close the
git backend process' stdin. So implement it only on
PackProxyServerProtocol and PackBackendProtocol.

HTTP and SSH still lack the ability to forward closed write connections,
but this simplifies that work. And SSH connections no longer hang if the
backend dies without telling the client it's done, as the dropped
connection will cause disconnections all the way up the stack.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~wgrant/turnip:half-close-rework into turnip:master.

diff --git a/turnip/pack/git.py b/turnip/pack/git.py
index a3becc3..2fbd59f 100644
--- a/turnip/pack/git.py
+++ b/turnip/pack/git.py
@@ -130,7 +130,6 @@ class PackProtocol(protocol.Protocol):
         self.transport.stopProducing()
 
 
-@implementer(IHalfCloseableProtocol)
 class PackProxyProtocol(PackProtocol):
 
     peer = None
@@ -144,18 +143,12 @@ class PackProxyProtocol(PackProtocol):
     def die(self, message):
         raise NotImplementedError()
 
-    def readConnectionLost(self):
-        if self.peer is not None:
-            self.peer.transport.loseWriteConnection()
-
-    def writeConnectionLost(self):
-        pass
-
     def connectionLost(self, reason):
         if self.peer is not None:
             self.peer.transport.loseConnection()
 
 
+@implementer(IHalfCloseableProtocol)
 class PackServerProtocol(PackProxyProtocol):
 
     got_request = False
@@ -201,6 +194,14 @@ class PackServerProtocol(PackProxyProtocol):
     def rawDataReceived(self, data):
         self.peer.sendRawData(data)
 
+    def readConnectionLost(self):
+        # Implementations need to forward the stdin down the stack,
+        # otherwise the backend will never know its work is done.
+        raise NotImplementedError()
+
+    def writeConnectionLost(self):
+        pass
+
     def connectionLost(self, reason):
         if reason.check(error.ConnectionDone):
             self.log.info('Connection closed.')
@@ -355,6 +356,11 @@ class PackProxyServerProtocol(PackServerProtocol):
                     self.command, self.pathname, self.params))
         PackServerProtocol.resumeProducing(self)
 
+    def readConnectionLost(self):
+        # Forward the closed stdin down the stack.
+        if self.peer is not None:
+            self.peer.transport.loseWriteConnection()
+
 
 class PackBackendProtocol(PackServerProtocol):
     """Filesystem-backed turnip-flavoured Git pack protocol implementation.
@@ -401,13 +407,10 @@ class PackBackendProtocol(PackServerProtocol):
         reactor.spawnProcess(self.peer, cmd, args, env=env)
 
     def readConnectionLost(self):
+        # Forward the closed stdin down the stack.
         if self.peer is not None:
             self.peer.loseWriteConnection()
 
-    def writeConnectionLost(self):
-        if self.peer is not None:
-            self.peer.loseReadConnection()
-
     def connectionLost(self, reason):
         if self.hookrpc_key:
             self.factory.hookrpc_handler.unregisterKey(self.hookrpc_key)
diff --git a/turnip/pack/ssh.py b/turnip/pack/ssh.py
index 3af7811..ebbf86a 100644
--- a/turnip/pack/ssh.py
+++ b/turnip/pack/ssh.py
@@ -30,7 +30,6 @@ from twisted.internet.error import (
     ConnectionDone,
     ProcessTerminated,
     )
-from twisted.internet.interfaces import IHalfCloseableProtocol
 from twisted.python import (
     components,
     failure,
@@ -53,7 +52,6 @@ __all__ = [
     ]
 
 
-@implementer(IHalfCloseableProtocol)
 class SSHPackClientProtocol(PackProtocol):
     """Bridge between a Git pack connection and a smart SSH request.
 
@@ -65,7 +63,6 @@ class SSHPackClientProtocol(PackProtocol):
     """
 
     def __init__(self):
-        self._closing = False
         self._closed = False
 
     def backendConnectionFailed(self, msg):
@@ -101,16 +98,6 @@ class SSHPackClientProtocol(PackProtocol):
     def rawDataReceived(self, data):
         self.factory.ssh_protocol.outReceived(data)
 
-    def readConnectionLost(self):
-        if self._closing:
-            self.connectionLost(ConnectionDone("Connection done"))
-        self._closing = True
-
-    def writeConnectionLost(self):
-        if self._closing:
-            self.connectionLost(ConnectionDone("Connection done"))
-        self._closing = True
-
     def connectionLost(self, reason):
         if not self._closed:
             self._closed = True