Opened 10 years ago

Closed 3 years ago

#745 defect closed fixed (fixed)

writeSequence not well-tested

Reported by: jknight Owned by: exarkun
Priority: high Milestone:
Component: core Keywords:
Cc: itamarst, jknight, jesstess Branch: branches/test-write-sequence-745-7
(diff, github, buildbot, log)
Author: exarkun, therve Launchpad Bug:

Description


Change History (30)

comment:1 Changed 10 years ago by jknight

r11925 broke writeSequence badly. The new writeSequence doesn't call startWriting, nor pause 
producers when too much is buffered. 

Since startWriting isn't called, if you just use writeSequence and not write, nothing will ever get written. 
Apparently our tests suck, because the tests don't catch this.

comment:2 Changed 10 years ago by itamarst

Fixed the behaviour, now it needs tests.

comment:3 Changed 6 years ago by therve

  • Owner changed from itamarst to therve

comment:4 Changed 6 years ago by therve

  • author set to therve
  • Branch set to branches/test-write-sequence-745

(In [23463]) Branching to 'test-write-sequence-745'

comment:5 Changed 6 years ago by therve

(In [23464]) Add tests for the 2 corner cases mentioned: no write and a streaming producer.

Refs #745

comment:6 Changed 6 years ago by therve

  • Keywords review added
  • Owner therve deleted

Please review.

comment:7 Changed 6 years ago by therve

  • Keywords review removed
  • Owner set to therve

I've seen a problem with iocp I think.

comment:8 Changed 6 years ago by therve

  • Keywords review added
  • Owner therve deleted

There was indeed a problem that I fixed.

comment:9 Changed 6 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

I waited just long enough before reviewing this so I could ask if you felt like writing these new tests in twisted/internet/test/test_tcp.py. ;)

This might also be an okay time for me to mention a couple things I'm not sure about with ReactorBuilder. One is that it doesn't make you do cleanup. Maybe this is okay for direct reactor tests, since the tests should really know what they're doing, and leaving stuff behind afterwards can be okay as long as you know you're doing it. The other is that ReactorBuilder tests don't support Deferred results as nicely as normal tests do. If you return a Deferred from one, trial will spin the wrong reactor for you. This isn't catastrophic, since you can start and stop your reactor inside one test method, but it does mean you don't get timeouts. Anyway, that's probably enough off-topic rambling.

Nice IOCP fix.

test_writeSequenceStreamingProducer and test_writeSequenceNonStreamingProducer might be problematic. It depends on the OS send-buffer size being less than 100000 bytes. Generally it is (although Linux gets close, 87380) but it'd be best if we could avoid relying on this. I'm not sure what I'd suggest doing instead. A fake could pretend to have a full buffer, but faking socket is a big job (and we'd want a verified fake). Maybe if the assumption being made is documented a bit more clearly (ie, that the socket send buffer is no bigger than 100000 bytes) it'll be fine.

comment:10 Changed 6 years ago by therve

  • Branch changed from branches/test-write-sequence-745 to branches/test-write-sequence-745-2

(In [23652]) Branching to 'test-write-sequence-745-2'

comment:11 Changed 6 years ago by therve

I've moved the tests. I wonder if the reactor manipulation is robust enough, but it works for now. I also solved a bug in tcp where global reactor was used.

Regarding the 100000 bytes in tests, they are calculated from the bufferSize of FileDescriptor, not from the OS limit. I've added a comment to explain that.

comment:12 Changed 6 years ago by therve

  • Branch changed from branches/test-write-sequence-745-2 to branches/test-write-sequence-745-3

(In [25016]) Branching to 'test-write-sequence-745-3'

comment:13 Changed 6 years ago by therve

  • Keywords review added
  • Owner therve deleted

I forgot why I didn't put this into review at that time, so I'll push it now.

comment:14 Changed 6 years ago by exarkun

  • Keywords review removed
  • Owner set to therve
  1. in twisted/internet/tcp.py, Server is changed so that reactor to its __init__ is optional, and the caller is changed to pass a reactor. Are both of these changes actually necessary? I dislike the former and like the latter.
  2. I'm confused about why either writeSequence or write has any code for dealing with a producer at all. Why isn't doWrite solely responsible for pausing and resuming a producer? It's the method which knows if the socket is actually accepting more data or not.
  3. Assuming there's a good reason for that, or at least that it's something that should be fixed separately from this ticket, there's a ton of duplication here. Four implementations of the producer pausing logic, only three of which are correct even with the fix in this branch:
    1. twisted.internet.abstract.FileDescriptor.write
    2. twisted.internet.abstract.FileDescriptor.writeSequence
    3. twisted.internet.iocpreactor.abstract.FileDescriptor.write
    4. twisted.internet.iocpreactor.abstract.FileDescriptor.writeSequence
  4. The tests should use ReactorBuilder.runReactor
  5. Since the testcase is named WriteSequenceTests, maybe the test methods don't need writeSequence in their names.
  6. There are failures on Windows (not even counting IOCP builders). :( http://buildbot.twistedmatrix.com/builders/server-2k8-x86-py2.5-select/builds/12

comment:15 Changed 5 years ago by therve

  • Keywords review added
  • Owner therve deleted

OK, this is back to review. Builders look green enough. I think this ticket will trigger some tickets, but I'd like to defer this:

  • remove the duplication mentioned above
  • do something about bufferSize versus writeBufferSize

comment:16 Changed 5 years ago by exarkun

  • Keywords review removed

Cool. Please file tickets for those two points, add a comment in the code referring to each of those tickets, and then merge.

comment:17 Changed 5 years ago by exarkun

  • Owner set to therve

Bump.

comment:18 Changed 5 years ago by therve

The new tests I wrote are affected by the same bug as #3925. So merging this would introduce 8 new test failures under Windows...

comment:19 Changed 5 years ago by exarkun

Achtung.

comment:20 Changed 4 years ago by jesstess

  • Cc jesstess added

comment:21 Changed 4 years ago by therve

  • Branch changed from branches/test-write-sequence-745-3 to branches/test-write-sequence-745-4

(In [28544]) Branching to 'test-write-sequence-745-4'

comment:22 Changed 4 years ago by therve

Those tests are still broken under Windows...

comment:23 Changed 4 years ago by exarkun

  • Author changed from therve to exarkun, therve
  • Branch changed from branches/test-write-sequence-745-4 to branches/test-write-sequence-745-5

(In [30270]) Branching to 'test-write-sequence-745-5'

comment:24 Changed 3 years ago by <automation>

  • Owner therve deleted

comment:25 Changed 3 years ago by exarkun

  • Branch changed from branches/test-write-sequence-745-5 to branches/test-write-sequence-745-6

(In [32682]) Branching to 'test-write-sequence-745-6'

comment:26 Changed 3 years ago by exarkun

(In [32691]) Rewrite the new writeSequence/producer tests to avoid #5285

refs #745
refs #5285

comment:27 Changed 3 years ago by exarkun

  • Keywords review added

I merged this forward and resolved some simple conflicts. I also changed test_streamingProducer and test_nonStreamingProducer to work - they were missing basic setup, like listenTCP and connectTCP, so I guess they were left in a slightly incomplete state last time this was worked on. Then I changed them around a bit to avoid triggering #5285.

Build results look pretty good now.

comment:28 Changed 3 years ago by glyph

  • Keywords review removed
  • Owner set to exarkun

Looks like there's still one more trivial conflict, but otherwise this branch looks fabulous. Love the IOCP fix; I wonder if it will make future builds more reliable?

Please land.

comment:29 Changed 3 years ago by exarkun

  • Branch changed from branches/test-write-sequence-745-6 to branches/test-write-sequence-745-7

(In [32736]) Branching to 'test-write-sequence-745-7'

comment:30 Changed 3 years ago by exarkun

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

(In [32738]) Merge test-write-sequence-745-7

Author: therve, exarkun
Reviewer: exarkun, glyph
Fixes: #745

Add ReactorBuilder-style unit tests for ITransport.writeSequence. Fix a bug
thusly revealed in IOCP reactor's implementation of that method when combined
with a pull producer.

Note: See TracTickets for help on using tickets.