[WIP] Fix RIO Thread #1420
[WIP] Fix RIO Thread #1420
Conversation
| var task = PongServer(connection); | ||
| } | ||
|
|
||
| server.Stop(); |
There was a problem hiding this comment.
Is this a race with PongServer, which isn't awaited? I didn't get any issues running this, but it could still be sketchy...
|
The issue is RIO isn't backed by winsock buffers and FlushAsync may take time so you need to submit another receive before it else the NIC will drop the next packet and you'll hit a TCP re-transmit instead; which is fine if FlushAsync is actually blocking but not good if it just takes a little time (i.e isn't dispatched to a different thread). |
| _input.Writer.Complete(); | ||
| } | ||
| else | ||
| // REVIEW: Why was there special behaviour for bytesTransferred = 0? |
There was a problem hiding this comment.
bytesTransferred == 0 is end of data/pipe complete
Isn't it the thing that's triggering the recieve completes? (edit) maybe not; might need to see what the code is doing agian |
|
Ok so I've updated this based on our chat on gitter. I couldn't confirm it though on current tooling because I broke my environment updating to 2.0... And we can't run the test yet with the new I might shelve this until we can run the tests and confirm it all works. |
|
Turned out the I don't think we'll be able to get away with deferring the flush. Looks like it has to happen before |
|
@benaadams This works: public void ReceiveBeginComplete(uint bytesTransferred)
{
if (bytesTransferred == 0)
{
_buffer.FlushAsync();
_input.Writer.Complete();
}
else
{
_buffer.Advance((int)bytesTransferred);
_buffer.Commit();
_buffer.FlushAsync();
ProcessReceives();
}
}
public void ReceiveEndComplete()
{
}I'm wondering if the issue of potentially blocking on |
Its the Which can be a synchronous execution of everything upstream of the pipe |
|
|
|
@ahsonkhan Yeh that's the failstack. We haven't got the state transitions right. @benaadams Oh I see. Is that synchronous at the scheduling level, or at the underlying IO? Could we look at an alternative scheduler for the reader? Otherwise I don't really see an alternative without changes to pipelines. |
|
@dotnet-bot test this please |
That's kind of weird, it's trying to dispose the libuv loop while it's still busy doing io, and I haven't directly touched any libuv code in this PR. If #1432 passes I'll just rebase on that and use its approach to |
Sounds good. @shiftylogic will work on resolving the issue with #1432. |
|
@KodrAus, please resolve the conflict. @davidfowl, review? |
|
@benaadams Do you have some new code in Kestrel that works around this? If so it's probably best for me to just close this and we can port that over. Otherwise I'll fix the conflict and remove dead code. |
|
Yeah aspnet/KestrelHttpServer#1630 but its quite a change as moved to threadpool and events rather than completion ports; would go with this change. Still haven't resolved the flushing completely as the read flushing needs some seperation |
|
Ok I'll polish it up today then and ping when it's ready for review 👍 |
|
@KodrAus open a new PR when ready |
There's an issue with the RIO code calling
_buffer.FlushAsync()inReceiveEndCompleteafter it's beenreallocatedput back in a writable state byReceiveBeginComplete.To work around this I've just removed the
ReceiveEndCompletemethod and am flushing before updating the shared input buffer.I haven't removed any of the notify infrastructure in
RioThread, even though it currently does nothing, because I thought we could work out what to do with it here. Maybe it can just be deleted, or some other work could be shifted onto the notify thread. Not sure.The test is blocked on by-ref stuff, but this same code change worked fine pre-2.0 so it should work once that's sorted.
CC: @davidfowl @benaadams @aL3891