Ticket #745 (new defect )

Opened 4 years ago

Last modified 4 weeks ago

writeSequence not well-tested

Reported by: jknight Assigned to: therve
Type: defect Priority: high
Milestone: Component: core
Keywords: Cc: itamarst, jknight
Branch: branches/test-write-sequence-745-3 Author: therve
Launchpad Bug:

Attachments

Change History

  2004-10-11 23:35:59+00:00 changed 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.

  2004-10-12 01:08:58+00:00 changed by itamarst 

Fixed the behaviour, now it needs tests.

  2008-04-28 16:23:02+00:00 changed by therve

  • owner changed from itamarst to therve
  • branch deleted
  • author deleted

  2008-04-30 13:01:13+00:00 changed by therve

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

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

  2008-04-30 13:03:27+00:00 changed by therve

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

Refs #745

  2008-04-30 13:03:57+00:00 changed by therve

  • keywords set to review
  • owner deleted

Please review.

  2008-04-30 13:22:51+00:00 changed by therve

  • keywords deleted
  • owner set to therve

I've seen a problem with iocp I think.

  2008-04-30 13:55:05+00:00 changed by therve

  • keywords set to review
  • owner deleted

There was indeed a problem that I fixed.

  2008-05-17 22:04:45+00:00 changed by exarkun

  • keywords deleted
  • 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.

  2008-05-18 12:20:11+00:00 changed 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'

  2008-05-18 13:59:10+00:00 changed 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.

  2008-10-22 16:15:40+00:00 changed 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'

  2008-10-22 16:23:51+00:00 changed by therve

  • keywords set to review
  • owner deleted
  • launchpad_bug deleted

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

  2008-10-23 14:41:16+00:00 changed by exarkun

  • keywords deleted
  • 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
Note: See TracTickets for help on using tickets.