writeSequence not well-tested

[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.

[itamarst]

Fixed the behaviour, now it needs tests.

[therve]

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

[therve]

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

Refs #745

[therve]

Please review.

[therve]

I've seen a problem with iocp I think.

[therve]

There was indeed a problem that I fixed.

[exarkun]

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.

[therve]

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

[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.

[therve]

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

[therve]

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

[exarkun]

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

[therve]

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

[exarkun]

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

[exarkun]

Bump.

[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...

[exarkun]

Achtung.

[therve]

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

[therve]

Those tests are still broken under Windows...

[exarkun]

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

[exarkun]

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

[exarkun]

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

refs #745 refs #5285

[exarkun]

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.

[glyph]

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.

[exarkun]

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

[exarkun]

(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.