Opened 5 years ago

Closed 8 weeks 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

Description

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 5 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 5 years ago by zooko

Cc: zooko@… added
Keywords: review added

comment:2 Changed 4 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 4 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 4 years ago by Tom Prince

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

Refs: #6557

comment:5 Changed 4 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 4 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.py?rev=39105#L559 PosixBase._doReadOrWrite]) although this behavior is deprecated (see [source:trunk/twisted/internet/tcp.py?rev=39105#L247 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 4 years ago by Tom Prince

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

comment:8 Changed 8 weeks ago by mark williams

Keywords: review added

comment:9 Changed 8 weeks ago by Moshe Zadka

Keywords: review removed

comment:10 Changed 8 weeks ago by Mark Williams <mrw@…>

Resolution: fixed
Status: newclosed

In 7cca42b2:

Merge pull request #918 from twisted/line-only-receiver-connection-lost-6557

Line only receiver connection lost 6557

Author: zooko, markrwilliams
Reviewer: moshez
Fixes: ticket:6557

LineOnlyReceiver disconnects the transport after receiving a line that exceeds MAX_LENGTH, like LineReceiver.

Note: See TracTickets for help on using tickets.