Opened 8 years ago

Closed 4 years ago

#6557 defect closed fixed (fixed)

LineOnlyReceiver doesn't disconnect the transport when it detects an overlarge line

Reported by: zooko Owned by: zooko
Priority: normal Milestone:
Component: core Keywords:
Cc: zooko@… Branch: branches/line-only-receiver-connection-lost-6557
branch-diff, diff-cov, branch-cov, buildbot
Author: tomprince


This patch and test make LineOnlyReceiver disconnect the transport, in the same way that LineReceiver does.

Attachments (1)

LineOnlyReceiver-disconnect-on-overlarge-line.patch (2.7 KB) - added by zooko 8 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 8 years ago by zooko

Cc: zooko@… added
Keywords: review added

comment:2 Changed 8 years 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:3 Changed 8 years ago by zooko

Sorry, and also this ticket, #6557, is a bugfix ticket that in my opinion ought to be merged separately from and before any solution to #6357.

comment:4 Changed 8 years ago by Tom Prince

(In [39126]) Apply LineOnlyReceiver-disconnect-on-overlarge-line.patch from zooko.

Refs: #6557

comment:5 Changed 8 years ago by Tom Prince

Author: tomprince
Branch: branches/line-only-receiver-connection-lost-6557

(In [39125]) Branching to line-only-receiver-connection-lost-6557.

comment:6 Changed 8 years ago by Tom Prince

Keywords: review removed
Owner: set to zooko

Note that this doesn't actually change the observable behavior. Returning ConnectionLost cause a disconnection (see [source:trunk/twisted/internet/ PosixBase._doReadOrWrite]) although this behavior is deprecated (see [source:trunk/twisted/internet/ tcp.Connection._dataReceived]). Since the behavior is deprecated, I think changing the implementation is reasonable, particularly if it makes refactoring easier.

  1. This change will need a new file (see ReviewProcess. I guess either a .removal mentioning the changed behaviour of lineLengthExceeded or a .misc.
  2. The test doesn't seem to be testing the edge case (MAX_LENGHT + 1). Is this because of a different bug? If so, could you mention that, and provide a link to the ticket?

Thanks for working on this. Please resubmit for review (with a patch against the branch) after adding a news file and addressing 2.

p.s. You appear to have commit access, so you could also just work directly on the branch.

comment:7 Changed 8 years ago by Tom Prince

Also, there are some pyflakes and twistedchecker errors (here).

comment:8 Changed 4 years ago by mark williams

Keywords: review added

comment:9 Changed 4 years ago by Moshe Zadka

Keywords: review removed

comment:10 Changed 4 years ago by Mark Williams <mrw@…>

Resolution: fixed
Status: newclosed

In 7cca42b2:

Error: Processor CommitTicketReference failed
 does not appear to be a Git repository. See the log for more information.
Note: See TracTickets for help on using tickets.