[#9446] iocp reactor is duplicating/loosing outgoing data #1025
Conversation
Codecov Report
@@ Coverage Diff @@
## trunk #1025 +/- ##
==========================================
- Coverage 91.93% 91.57% -0.36%
==========================================
Files 846 846
Lines 151297 151301 +4
Branches 13196 13196
==========================================
- Hits 139093 138558 -535
- Misses 10107 10619 +512
- Partials 2097 2124 +27 |
|
This looks good to me, although I'm not familiar with the IOCP API at all. To make sure I understand the code, it looks like (If so, perhaps it's worth fixing it in a separate PR so that this one can be committed…) |
|
fwiw, would be cool to land this, as the |
| "client didn't receive all the data it expected " | ||
| "(%d != %d)" % (clientF.len, self.datalen)) | ||
| self.assertTrue(clientF.len <= self.datalen, |
There was a problem hiding this comment.
What is the point of this assertion?
Why the previous one was not good? :)
There was a problem hiding this comment.
Current IOCP reactor shows rather interesting behaviour. ~10% of runs are ok, ~80% will loose data, and the remaining ~10% return duplicate data. Of course a simple == assertion would do, but the error message should not state data is missing.
There was a problem hiding this comment.
maybe the assertion message can be changed from didn't received all the data to didn't receive the expected data ... and replace with the dedicated self.assertEqual
but this is minor comment ...
what I would like to see, is this tests executed without the patch applied and see that it constantly fails
and then when applying the patch we have a solid green test :)
There was a problem hiding this comment.
Making test fail with probability 1 on the unpatched IOCP reactor will be hard IMHO. The issue is with the interaction of twisted with the Windows network stack. You had to mock that to get deterministic behaviour. You could run the test multiple times which would obviously reduce chances of false passes.
That said, this is the commit that changed the test. Everything else (including the IOCP reactor) is still unchanged. You might give that a spin to get an idea of the frequency of false passes ...
There was a problem hiding this comment.
A mock that emulates the behavior of the IOCP APIs under their various failure modes would be a welcome addition, especially to help other maintainers who may want to clean things up but don't know as much about IOCP. That said, it sounds like that would be a good followup.
src/twisted/test/test_tcp.py
Outdated
| self.transport.loseConnection() | ||
| self.transport.write(b'X'*(self.factory.len-1)) | ||
|
|
||
| def finish(): # write last byte separately to check issue #9446 |
There was a problem hiding this comment.
I guess that the inline command can be set as a docstring for this function.
Rather than "check issue #9446", maybe this command can describe the targeted behaviour regardless of the ticket number.
src/twisted/test/test_tcp.py
Outdated
| @@ -1363,15 +1363,21 @@ def test_buildProtocolClient(self): | |||
|
|
|||
|
|
|||
| class LargeBufferWriterProtocol(protocol.Protocol): | |||
|
|
|||
| # Win32 sockets cannot handle single huge chunks of bytes. Write one | |||
| # massive string to make sure Twisted deals with this fact. | |||
|
|
|||
| def connectionMade(self): | |||
| # write 60MB | |||
There was a problem hiding this comment.
do we still write 60MB? it looks like there are 60MB-1 ... I guess that we can remove this comment
|
cool, thanks for merging! also pinged OP to check if the issue is actually gone as it surfaced at the user level (crossbar in this case) and |
|
It sounds like we've gotten lots of feedback on this and it improves the situation, everybody feels like it's good, but nobody wants to approve it. Sounds like it's time for somebody to be the hero that Twisted needs. |
iocpreactor.abstract.FileHandle.doWrite()can get called again before._handleWrite()got the chance to update the bytes already written if.write()gets called while a.writeToHandle()is still in flight..startWriting()to only schedule a call to._resumeWriting()whenself.writing == False.test_tcp.LargeBufferTestswrite the last byte in a separate.write().