Opened 16 months ago

Last modified 14 months ago

#6556 enhancement new

LineReceiver rejects lines of length MAX_LENGTH sometimes

Reported by: zooko Owned by: zooko
Priority: normal Milestone:
Component: core Keywords:
Cc: zooko@… Branch: branches/line-receiver-max-length-fragmentation-6556
(diff, github, buildbot, log)
Author: tomprince Launchpad Bug:

Description

If MAX_LENGTH is 3 and delimiter is '\r\n', and I send "xyz\r" and then "\n" in two packets, LineReceiver will disconnect after receiving the first packet, calling lineLengthExceeded. On the other hand if I send "xyz\r\n" in one packet, LineReceiver will accept the line.

Here is a test. I propose a fix: LineReceiver should not check for line-length-exceeded until the number of bytes of the data >= MAX_LENGTH + len(delimiter).

Attachments (4)

notQuiteMaximumLineLengthUnfinished.patch (1.2 KB) - added by zooko 16 months ago.
test-max-length.patch (20.8 KB) - added by zooko 16 months ago.
fix-max-length.patch (21.6 KB) - added by zooko 16 months ago.
fix-max-length.2.patch (3.6 KB) - added by zooko 16 months ago.

Download all attachments as: .zip

Change History (13)

Changed 16 months ago by zooko

comment:1 Changed 16 months ago by zooko

Here are some more tests which I developed while working on optimizations for LineReceiver (#6357). Some of these tests detected bugs in some of my attempted optimizations, so I consider them useful. Look out for the one disabled, and accompanied by a comment saying "I propose we don't require this behavior.". I'm including it in this patch to solicit opinions from others — do you agree that behavior shouldn't be required?

comment:2 Changed 16 months ago by zooko

  • Cc zooko@… added

Changed 16 months ago by zooko

Changed 16 months ago by zooko

comment:3 Changed 16 months ago by zooko

  • Keywords review added

Here is a patch with just the test and the fix for just this issue. Please review!

comment:4 Changed 15 months ago by zooko

Please see my comment over on comment:4:ticket:6536, which is that I have a working patch for #6357 and am blocked because I would like for that optimization patch to be applied separately, on top of these bugfix patches: #6556, #6537, #6558.

comment:5 Changed 15 months ago by zooko

Please see my comment over on comment:4:ticket:6536, which is that I have a working patch for #6357 and am blocked because I would like for that optimization patch to be applied separately, on top of these bugfix patches: #6556, #6537, #6558.

comment:6 Changed 14 months ago by tomprince

  • Author set to tomprince
  • Branch set to branches/line-receiver-max-length-fragmentation-6556

(In [39148]) Branching to line-receiver-max-length-fragmentation-6556.

comment:7 Changed 14 months ago by tomprince

(In [39149]) Apply fix-max-length.2.patch from zooko.

Refs: #6556

comment:8 Changed 14 months ago by tom.prince

I guess this will behave pathologically, when the delimiter is pathologically long. If it is a significant fraction of MAX_LENGTH, then MAX_LENGTH + len(delimiter) will be read, even if none of the characters after MAX_LENGTH match the delimiter. I dont think it worth worrying about this though.

comment:9 Changed 14 months ago by tom.prince

  • Keywords review removed
  • Owner set to zooko
  1. There is a python3 failure. It appears due to setting the delimiter explicitly to a string (rather than bytestring).
  2. There are some whitespace and line-length issues reported by twistedchecker.
  3. The following assertion won't provide a good error, in the case proto.received is []. Please split it into an assertion about len(proto.received) and proto.received[0].
    self.assertEqual(line, proto.received and proto.received[0])
    
  4. It appears there isn't any test of receiving a line as long as or longer than MAX_LENGTH + len(delimter) without a delimiter.
    • Please add one or more.
  5. Is there any reason you can't use LineTester? The extra stuff that it does doesn't appear to affect your tests.
    • Please switch to using it, or explain why it isn't sufficient.

Please resubmit for review, after addressing the above points.

Note: See TracTickets for help on using tickets.