Description

Change History (30)

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.

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.

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.

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.

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:

twisted.internet.abstract.FileDescriptor.write

twisted.internet.abstract.FileDescriptor.writeSequence

twisted.internet.iocpreactor.abstract.FileDescriptor.write

twisted.internet.iocpreactor.abstract.FileDescriptor.writeSequence

The tests should use ReactorBuilder.runReactor

Since the testcase is named WriteSequenceTests, maybe the test methods don't need writeSequence in their names.

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.