Update to mainline BestHTTP #9
Conversation
| // but in any case we want to close the stream to indicate to grpc that we are done | ||
| else | ||
| { | ||
| incomingDataStream.Close(); |
There was a problem hiding this comment.
we can't close the data stream as part of concluding the request, as the download stream may still be draining in the background. so the download process is responsible for closing the stream on the happy path now. any exception paths can still close the stream with an exception though
There was a problem hiding this comment.
except in the case of a trailers only response where we get no DATA, that will still close the stream
| { | ||
| if (m_Exception != null) | ||
| throw m_Exception; | ||
|
|
||
| // Either we have data to read, or we got flushed (e.g. stream got closed) | ||
| // or we are in non blocking read mode. | ||
| return Length >= count && m_Flushed || m_Closed || NonBlockingRead; | ||
| return m_Buffer.Count > 0 || m_Flushed || m_Closed || NonBlockingRead; |
There was a problem hiding this comment.
gRPC still expects the standard .NET stream semantics of: only returning from a Read with at least 1 byte of data, unless we reached EOF/EOS (in our case, flush/close), then we can return 0 or negatives. Returning partial reads (n < count) is still fine and handled appropriately in grpc.net.
There was a problem hiding this comment.
and it ultimately doesnt matter which side holds the buffer, but just felt better to me to drain this man-in-the-middle stream as fast as possible
| } | ||
|
|
||
| public override long Length => m_Buffer.Count; | ||
| // Avoid sending content-length=0 (EOF) by using -2 for chunked encoding. | ||
| public override long Length => -2; |
There was a problem hiding this comment.
We can leave this at -2 and Best HTTP will just stay in streaming/unbounded data mode, and will treat a -1 from Read as the EOF/EOS
|
|
||
| namespace GRPC.NET | ||
| { | ||
| public class PushPullStream : Stream | ||
| public class PushPullStream : UploadStreamBase |
There was a problem hiding this comment.
new base class for the uploader which handles receiving the thread signal on its own (although we still need to call it in Flush)
| else | ||
| { | ||
| // The DownloadContentStream does not block, so wait a bit. | ||
| await Task.Delay(ASYNC_READ_WAIT_MS, cancellationToken); |
There was a problem hiding this comment.
never happy about polling delays, but it is what it is. havent seen any other available notice/signal pattern for downloads in Best HTTP, but I may have missed it.
| incomingDataStream.Write(fragment, 0, length); | ||
| incomingDataStream.Flush(); | ||
| return true; | ||
| try |
There was a problem hiding this comment.
The new download streaming pattern:
- drain all available download at the time of this callback
- if the stream isn't done, kick off a Task and poll for new data until complete. The new "detatched" pattern will hold resources open until we finish draining and close the stream
|
|
||
| for (; readLength < count && Length > 0 && m_Buffer.Count > 0; readLength++) | ||
| { | ||
| for (; readLength < count && m_Buffer.Count > 0; readLength++) | ||
| buffer[readLength] = m_Buffer.Dequeue(); |
There was a problem hiding this comment.
offset needs to be added here:
buffer[offset+readLength] = ...just got bit by that bug today...
There was a problem hiding this comment.
Hi @colinrgodsey, thanks for also finding this bug. :-) I assume this happens for larger messages or how could we trigger this with a new testcase? For the initial version I tried to add a test-case for each bug I encountered.
Personally I only used high frequent short messages in production and testing until now.
There was a problem hiding this comment.
definitely a good idea keeping test cases for encountered bugs! not sure what exact internal behavior triggered this, but it did happen with a message that was larger than 128KB. i do fear the exact issue may have had to with some concurrent behavior (triggering a read on an existing buffer that hasnt been completely consumed/drained yet). although it was rather consistent in our case with this exact payload.
Use offset in `Read`
|
@doctorseus we've had this in production for a quite a while now with no issues. think its all looking solid |
BestHTTP/2 has been discontinued, and the support has been folded into mainline BestHTTP. The APIs are a little different, so had to upgrade them, but its working fine for us so far after a decent amount of torture testing.