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.

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

There is some obvious intermediate debugging cruft left in this patch that no doubt needs to be cleaned up.

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.

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.

There are some variables in the unit test that aren't named according to the naming convention. There are also some whitespace issues.

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?

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.

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.