Merge ~pappacena/turnip:hookrpc-networking-fixes into turnip:master

Proposed by Thiago F. Pappacena
Status: Merged
Approved by: Thiago F. Pappacena
Approved revision: 848df674ac7bc86451dd719923487495fbfba6a9
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~pappacena/turnip:hookrpc-networking-fixes
Merge into: turnip:master
Diff against target: 120 lines (+46/-6)
3 files modified
turnip/pack/hookrpc.py (+2/-0)
turnip/pack/hooks/hook.py (+9/-3)
turnip/pack/tests/test_hooks.py (+35/-3)
Reviewer Review Type Date Requested Status
Colin Watson Approve
Review via email: mp+384848@code.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/turnip/pack/hookrpc.py b/turnip/pack/hookrpc.py
2index 8c99e76..ec62672 100644
3--- a/turnip/pack/hookrpc.py
4+++ b/turnip/pack/hookrpc.py
5@@ -41,6 +41,8 @@ from turnip.pack.helpers import (
6
7 class JSONNetstringProtocol(basic.NetstringReceiver):
8 """A protocol that sends and receives JSON as netstrings."""
9+ # 200MB should be enough.
10+ MAX_LENGTH = 200 * 1024 * 1024
11
12 def stringReceived(self, string):
13 try:
14diff --git a/turnip/pack/hooks/hook.py b/turnip/pack/hooks/hook.py
15index e135395..0dba4dd 100755
16--- a/turnip/pack/hooks/hook.py
17+++ b/turnip/pack/hooks/hook.py
18@@ -106,21 +106,27 @@ def match_update_rules(rule_lines, ref_line):
19
20
21 def netstring_send(sock, s):
22- sock.send(b'%d:%s,' % (len(s), s))
23+ sock.sendall(b'%d:%s,' % (len(s), s))
24
25
26 def netstring_recv(sock):
27 c = sock.recv(1)
28 lengthstr = b''
29 while c != b':':
30- assert c.isdigit()
31+ if not c.isdigit():
32+ raise ValueError(
33+ "Invalid response: %s" % (six.ensure_text(c + sock.recv(256))))
34 lengthstr += c
35 c = sock.recv(1)
36 length = int(lengthstr)
37 s = bytearray()
38 while len(s) < length:
39 s += sock.recv(length - len(s))
40- assert sock.recv(1) == b','
41+ ending = sock.recv(1)
42+ if ending != b',':
43+ raise ValueError(
44+ "Length error for message '%s': ending='%s'" %
45+ (six.ensure_text(bytes(s)), six.ensure_text(ending)))
46 return bytes(s)
47
48
49diff --git a/turnip/pack/tests/test_hooks.py b/turnip/pack/tests/test_hooks.py
50index 449b9f9..a98f16b 100644
51--- a/turnip/pack/tests/test_hooks.py
52+++ b/turnip/pack/tests/test_hooks.py
53@@ -88,9 +88,24 @@ class MockRepo(object):
54
55
56 class MockSocket(object):
57+ # "sends" up to this amount of bytes on "send()"
58+ MAX_SEND_DATA = 5
59
60 def __init__(self, blocks=None):
61 self.blocks = blocks or []
62+ self._sent_data = b""
63+
64+ def send(self, data, flags=None):
65+ to_send = data[:self.MAX_SEND_DATA]
66+ self._sent_data += to_send
67+ return min(self.MAX_SEND_DATA, len(data))
68+
69+ def sendall(self, data, flags=None):
70+ ret = self.send(data, flags)
71+ if ret > 0:
72+ return self.sendall(data[ret:], flags)
73+ else:
74+ return None
75
76 def recv(self, bufsize, flags=None):
77 if not self.blocks:
78@@ -108,15 +123,21 @@ class TestNetstringRecv(TestCase):
79
80 def test_nondigit(self):
81 sock = MockSocket([b'zzz:abc,'])
82- self.assertRaises(AssertionError, hook.netstring_recv, sock)
83+ self.assertRaisesRegex(
84+ ValueError, "Invalid response: zzz:abc,",
85+ hook.netstring_recv, sock)
86
87 def test_short(self):
88 sock = MockSocket([b'4:abc,'])
89- self.assertRaises(AssertionError, hook.netstring_recv, sock)
90+ self.assertRaisesRegex(
91+ ValueError, "Length error for message 'abc,': ending=''",
92+ hook.netstring_recv, sock)
93
94 def test_unterminated(self):
95 sock = MockSocket([b'4:abcd'])
96- self.assertRaises(AssertionError, hook.netstring_recv, sock)
97+ self.assertRaisesRegex(
98+ ValueError, "Length error for message 'abcd': ending=''",
99+ hook.netstring_recv, sock)
100
101 def test_split_data(self):
102 sock = MockSocket([b'12:abcd', b'efgh', b'ijkl,'])
103@@ -131,6 +152,17 @@ class TestNetstringRecv(TestCase):
104 hook.netstring_recv(sock))
105 self.assertEqual([b'remaining'], sock.blocks)
106
107+ def test_send_uses_sendall(self):
108+ sock = MockSocket([])
109+
110+ # Send up to 5 bytes on each "send()" call.
111+ # This is important to make sure we are using python's higher level
112+ # socket.sendall() rather than socket.send().
113+ sock.MAX_SEND_DATA = 5
114+
115+ hook.netstring_send(sock, b"some-fake-data")
116+ self.assertEqual(b"14:some-fake-data,", sock._sent_data)
117+
118
119 class HookTestMixin(object):
120 run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=5)

Subscribers

People subscribed via source and target branches