Opened 14 months ago

Closed 11 months ago

Last modified 11 months ago

#6536 defect closed fixed (fixed)

LineReceiver doesn't pass correct "remnants of buffer" to lineLengthExceeded()

Reported by: zooko Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords:
Cc: zooko@… Branch: branches/linglengthexceeded-buffer-6536
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description (last modified by zooko)

I sort of wish LineReceiver wasn't offering to tell lineLengthExceeded() the entire remnant of the data in its internal buffer. But since it is...

=== modified file 'twisted/protocols/basic.py'
--- twisted/protocols/basic.py  2013-04-18 13:15:14 +0000
+++ twisted/protocols/basic.py  2013-06-01 03:06:41 +0000
@@ -565,7 +565,7 @@
                     else:
                         lineLength = len(line)
                         if lineLength > self.MAX_LENGTH:
-                            exceeded = line + self._buffer
+                            exceeded = line + self.delimiter + self._buffer
                             self._buffer = b''
                             return self.lineLengthExceeded(exceeded)
                         why = self.lineReceived(line)

=== modified file 'twisted/protocols/test/test_basic.py'
--- twisted/protocols/test/test_basic.py        2013-04-18 13:15:14 +0000
+++ twisted/protocols/test/test_basic.py        2013-06-01 03:05:30 +0000
@@ -326,6 +326,42 @@
         self.assertTrue(transport.disconnecting)
 
 
+    def test_lineLengthExceeded(self):
+        """
+        C{LineReceiver} calls C{lineLengthExceeded} with the entire
+        remaining contents of its buffer.
+        """
+        caught_line = []
+        class ExcessivelyLargeLineCatcher(basic.LineReceiver):
+            def lineReceived(self, line):
+                pass
+            def lineLengthExceeded(self, line):
+                caught_line.append(line)
+
+        proto = ExcessivelyLargeLineCatcher()
+        proto.MAX_LENGTH=6
+        transport = proto_helpers.StringTransport()
+        proto.makeConnection(transport)
+        excessive_input = b'x' * (proto.MAX_LENGTH * 2 + 2)
+        proto.dataReceived(excessive_input)
+        self.assertEqual(caught_line[0], excessive_input)
+        del caught_line[:]
+
+        excessive_input = b'x' * (proto.MAX_LENGTH * 2 + 2)
+        proto.dataReceived(b'x'+proto.delimiter + excessive_input)
+        self.assertEqual(caught_line[0], excessive_input)
+        del caught_line[:]
+
+        excessive_input = b'x' * (proto.MAX_LENGTH * 2 + 2) + proto.delimiter + b'x' * (proto.MAX_LENGTH * 2 + 2)
+        proto.dataReceived(excessive_input)
+        self.assertEqual(caught_line[0], excessive_input)
+        del caught_line[:]
+
+        excessive_input = b'x' * (proto.MAX_LENGTH * 2 + 2) + proto.delimiter + b'x' * (proto.MAX_LENGTH * 2 + 2) + proto.delimiter
+        proto.dataReceived(excessive_input)
+        self.assertEqual(caught_line[0], excessive_input)
+        del caught_line[:]
+
     def test_rawDataError(self):
         """
         C{LineReceiver.dataReceived} forwards errors returned by

I'll be happy to submit this patch as a svn branch or whatever if you prefer...

Attachments (2)

linereceiver-remnant-fix.patch (7.3 KB) - added by zooko 14 months ago.
my-twisted-patch.patch (2.5 KB) - added by zooko 11 months ago.

Download all attachments as: .zip

Change History (17)

Changed 14 months ago by zooko

comment:1 Changed 14 months ago by zooko

  • Description modified (diff)

comment:2 follow-up: Changed 13 months ago by tom.prince

This is addressed in #6357, which is also changing the code here. But the test here looks like something that should be included there.

comment:3 Changed 13 months ago by zooko

  • Cc zooko@… added

comment:4 in reply to: ↑ 2 Changed 13 months ago by zooko

Replying to tom.prince:

This is addressed in #6357, which is also changing the code here. But the test here looks like something that should be included there.

For what it is worth, I wrote this patch and unit test because I was working on an alternate patch for #6357, and I thought it would be easier for everyone to evaluate my patch and to merge fixes if I broke off this patch and its test into a separate ticket. So I suggest that this unit test and patch should be applied to trunk and then any solution to #6357 should be built on top of this fix. That way trunk will already have this bugfix in it even before it gets a patch for the performance issue (#6357) in.

Again, my motivation here is that I was writing a patch for #6357, and it started to turn into a large patch, and become harder for me to keep in my head. Of course that will also mean it is hard for a reviewer to get into their head. Therefore, I broke off this piece of it, which a reviewer could relatively easily read. I think it would be a mistake to reject this patch on the basis that a larger patch that is (hopefully) coming soon would subsume it.

For what it is worth, my alternate patch for #6357 is complete, passes all current unit tests plus several more unit tests that I added, and has undergone extensive benchmarking, profiling, and optimization. So I would be keen to submit my alternate patch for #6357. The blocker for me is that I think it will be a big and complex process to review it if it comes with all these bugfixes in addition to its primary purpose of optimization.

comment:5 Changed 11 months ago by exarkun

+1 smaller units of work

comment:6 Changed 11 months ago by exarkun

  • Keywords review removed
  • Owner set to zooko

Thanks for your work on this zooko. Sorry about the long turn-around. :(

  1. There is some obvious intermediate debugging cruft left in this patch that no doubt needs to be cleaned up.
  2. There seems to be a lot of implementation changes. Are these really all necessary to make the new test pass? The patch you supplied in the ticket summary makes me suspect it isn't.
  3. The new stringchain dependency sadly requires some discussion. At a minimum, the license will need to be changed before Twisted may depend on it. Hopefully this can be deferred to a later ticket, since it seems stringchain isn't actually required to make this new test pass in a reasonable way.
  4. There are some variables in the unit test that aren't named according to the naming convention. There are also some whitespace issues.
  5. The attached patch doesn't cover as many edge cases as the patch in the ticket summary. Did you decide those cases aren't important?
  6. Also please add a news fragment to the next patch.

Thanks again.

comment:7 Changed 11 months ago by zooko

  • Status changed from new to assigned

Hm, I seem to have attached the wrong patch as attachment:linereceiver-remnant-fix.patch. That attachment looks different from the in-line patch that I pasted into the original comment, and the latter looks more correct, and the latter doesn't have the problems that exarkun objected to in comment:6. My apologies! I'll double-check and submit the correct patch.

comment:8 Changed 11 months ago by zooko

Okay, here is the correct patch (which is indeed the one pasted into the original description). I followed https://twistedmatrix.com/trac/wiki/BasicGuideToContributingCode to produce this. I ran the unit tests with the part of the patch that adds a unit test but without the part of the patch that fixes the bug, confirmed that the test detects the bug, then applies the part of the patch that fixes the bug and re-ran the tests and confirmed that this made the test go from red to green.

Changed 11 months ago by zooko

comment:9 Changed 11 months ago by zooko

  • Keywords review added
  • Owner changed from zooko to exarkun
  • Status changed from assigned to new

comment:10 Changed 11 months ago by exarkun

  • Author set to exarkun
  • Branch set to branches/linglengthexceeded-buffer-6536

(In [39883]) Branching to 'linglengthexceeded-buffer-6536'

comment:11 Changed 11 months ago by exarkun

(In [39884]) Apply my-twisted-patch.patch

refs #6536

comment:12 Changed 11 months ago by exarkun

  • Keywords review removed

Thanks for the updated patch.

  1. The test is too long. It's really four different tests so it should be four test methods.
  2. Some of the names don't comply with the naming convention.
  3. This merits a bugfix news fragment.
  4. The implementation change looks nice and simple.

These are pretty straightforward changes. I'll go ahead and make them and then apply the changes. Thanks.

comment:13 Changed 11 months ago by exarkun

(In [39885]) Split up long test; adjust to follow coding standard; add news fragment.

refs #6536

comment:14 Changed 11 months ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(In [39888]) Merge linglengthexceeded-buffer-6536

Author: zooko, exarkun
Reviewer: exarkun
Fixes: #6536

Add better test coverage for the behavior of LineReceiver.lineLengthExceeded and
fix a bug in LineReceiver that caused bad data to be passed to that method in
some cases.

comment:15 Changed 11 months ago by zooko

Thanks for the review and the refactored tests and the news file patch!

Note: See TracTickets for help on using tickets.