launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/turnip:hookrpc-networking-fixes into turnip:master.

Commit message:
Fixing bugs on hookrpc communication protocol causing large pushes to stale, and small refactoring to add more verbose exceptions.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1868935 in turnip: "AssertionError in pre-receive hook"
  https://bugs.launchpad.net/turnip/+bug/1868935

For more details, see:
https://code.launchpad.net/~pappacena/turnip/+git/turnip/+merge/384848
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/turnip:hookrpc-networking-fixes into turnip:master.

diff --git a/turnip/pack/hookrpc.py b/turnip/pack/hookrpc.py
index 8c99e76..ec62672 100644
--- a/turnip/pack/hookrpc.py
+++ b/turnip/pack/hookrpc.py
@@ -41,6 +41,8 @@ from turnip.pack.helpers import (
 
 class JSONNetstringProtocol(basic.NetstringReceiver):
     """A protocol that sends and receives JSON as netstrings."""
+    # 200MB should be enough.
+    MAX_LENGTH = 200 * 1024 * 1024
 
     def stringReceived(self, string):
         try:
diff --git a/turnip/pack/hooks/hook.py b/turnip/pack/hooks/hook.py
index e135395..0dba4dd 100755
--- a/turnip/pack/hooks/hook.py
+++ b/turnip/pack/hooks/hook.py
@@ -106,21 +106,27 @@ def match_update_rules(rule_lines, ref_line):
 
 
 def netstring_send(sock, s):
-    sock.send(b'%d:%s,' % (len(s), s))
+    sock.sendall(b'%d:%s,' % (len(s), s))
 
 
 def netstring_recv(sock):
     c = sock.recv(1)
     lengthstr = b''
     while c != b':':
-        assert c.isdigit()
+        if not c.isdigit():
+            raise ValueError(
+                "Invalid response: %s" % (six.ensure_text(c + sock.recv(256))))
         lengthstr += c
         c = sock.recv(1)
     length = int(lengthstr)
     s = bytearray()
     while len(s) < length:
         s += sock.recv(length - len(s))
-    assert sock.recv(1) == b','
+    ending = sock.recv(1)
+    if ending != b',':
+        raise ValueError(
+            "Length error for message '%s': ending='%s'" %
+            (six.ensure_text(bytes(s)), six.ensure_text(ending)))
     return bytes(s)
 
 
diff --git a/turnip/pack/tests/test_hooks.py b/turnip/pack/tests/test_hooks.py
index 449b9f9..a98f16b 100644
--- a/turnip/pack/tests/test_hooks.py
+++ b/turnip/pack/tests/test_hooks.py
@@ -88,9 +88,24 @@ class MockRepo(object):
 
 
 class MockSocket(object):
+    # "sends" up to this amount of bytes on "send()"
+    MAX_SEND_DATA = 5
 
     def __init__(self, blocks=None):
         self.blocks = blocks or []
+        self._sent_data = b""
+
+    def send(self, data, flags=None):
+        to_send = data[:self.MAX_SEND_DATA]
+        self._sent_data += to_send
+        return min(self.MAX_SEND_DATA, len(data))
+
+    def sendall(self, data, flags=None):
+        ret = self.send(data, flags)
+        if ret > 0:
+            return self.sendall(data[ret:], flags)
+        else:
+            return None
 
     def recv(self, bufsize, flags=None):
         if not self.blocks:
@@ -108,15 +123,21 @@ class TestNetstringRecv(TestCase):
 
     def test_nondigit(self):
         sock = MockSocket([b'zzz:abc,'])
-        self.assertRaises(AssertionError, hook.netstring_recv, sock)
+        self.assertRaisesRegex(
+            ValueError, "Invalid response: zzz:abc,",
+            hook.netstring_recv, sock)
 
     def test_short(self):
         sock = MockSocket([b'4:abc,'])
-        self.assertRaises(AssertionError, hook.netstring_recv, sock)
+        self.assertRaisesRegex(
+            ValueError, "Length error for message 'abc,': ending=''",
+            hook.netstring_recv, sock)
 
     def test_unterminated(self):
         sock = MockSocket([b'4:abcd'])
-        self.assertRaises(AssertionError, hook.netstring_recv, sock)
+        self.assertRaisesRegex(
+            ValueError, "Length error for message 'abcd': ending=''",
+            hook.netstring_recv, sock)
 
     def test_split_data(self):
         sock = MockSocket([b'12:abcd', b'efgh', b'ijkl,'])
@@ -131,6 +152,17 @@ class TestNetstringRecv(TestCase):
             hook.netstring_recv(sock))
         self.assertEqual([b'remaining'], sock.blocks)
 
+    def test_send_uses_sendall(self):
+        sock = MockSocket([])
+
+        # Send up to 5 bytes on each "send()" call.
+        # This is important to make sure we are using python's higher level
+        # socket.sendall() rather than socket.send().
+        sock.MAX_SEND_DATA = 5
+
+        hook.netstring_send(sock, b"some-fake-data")
+        self.assertEqual(b"14:some-fake-data,", sock._sent_data)
+
 
 class HookTestMixin(object):
     run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=5)