[QUIC] Review comments #58608
Closed
[QUIC] Review comments #58608
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsNotes from yesterday.
|
ManickaP
added
NO-MERGE
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsNotes from yesterday.
|
ManickaP
commented
Sep 20, 2021
| @@ -483,11 +489,13 @@ internal override ValueTask<int> ReadAsync(Memory<byte> destination, Cancellatio | |||
| _state.ReadState = ReadState.None; | |||
|
|
|||
| int taken = CopyMsQuicBuffersToUserBuffer(_state.ReceiveQuicBuffers.AsSpan(0, _state.ReceiveQuicBuffersCount), destination.Span); | |||
| // CR: we shouldn't be calling msquic in a lock | |||
| ReceiveComplete(taken); | ||
|
|
||
| if (taken != _state.ReceiveQuicBuffersTotalBytes) | ||
| { | ||
| // Need to re-enable receives because MsQuic will pause them when we don't consume the entire buffer. | ||
| // CR: we shouldn't be calling msquic in a lock |
| @@ -354,6 +354,8 @@ private CancellationTokenRegistration HandleWriteStartState(bool emptyBuffer, Ca | |||
| } | |||
| }, _state); | |||
|
|
|||
| // CR: We should make sure that we never return from a final state (Abort etc.) back to writing. | |||
| @@ -354,6 +354,8 @@ private CancellationTokenRegistration HandleWriteStartState(bool emptyBuffer, Ca | |||
| } | |||
| }, _state); | |||
|
|
|||
| // CR: We should make sure that we never return from a final state (Abort etc.) back to writing. | |||
| // Also we're missing final state for writing (we have one for reading). | |||
Comment on lines
+385
to
+386
| // CR: Consolidate the state machine from so many small helpers (obstacle - 3 different write methods). | ||
| // Do not use Handle... for private helpers that are not msquic callbacks. |
| @@ -845,6 +861,7 @@ private void Dispose(bool disposing) | |||
|
|
|||
| private void EnableReceive() | |||
| { | |||
| // CR: assert no lock is taken | |||
| @@ -638,6 +649,8 @@ internal override async ValueTask ShutdownCompleted(CancellationToken cancellati | |||
| } | |||
| } | |||
|
|
|||
| // CR: should we be issuing abortive shutdown (immediate) here instead? Beware of shutdown event handler releasing memory and having | |||
| @@ -665,6 +678,7 @@ internal override ValueTask WaitForWriteCompletionAsync(CancellationToken cancel | |||
| return new ValueTask(_state.ShutdownWriteCompletionSource.Task.WaitAsync(cancellationToken)); | |||
| } | |||
|
|
|||
| // CR: naming ShutdownWrites, this one closes write side of the stream only | |||
| @@ -805,6 +819,7 @@ private void Dispose(bool disposing) | |||
| } | |||
|
|
|||
| // Check if we already got final event. | |||
| // CR: use constants for the values of ShutdownDone | |||
| @@ -825,6 +840,7 @@ private void Dispose(bool disposing) | |||
| { | |||
| try | |||
| { | |||
| // CR: Use a constant for 0xffffffff | |||
ManickaP
commented
Sep 20, 2021
| @@ -866,6 +883,8 @@ private void EnableReceive() | |||
| return HandleEvent(state, ref streamEvent); | |||
| } | |||
|
|
|||
| // CR: why is this a seperate function??? | |||
| @@ -883,6 +902,7 @@ private static uint HandleEvent(State state, ref StreamEvent evt) | |||
| return HandleEventStartComplete(state, ref evt); | |||
| // Received data on the stream | |||
| case QUIC_STREAM_EVENT_TYPE.RECEIVE: | |||
| // CR: naming Receive | |||
| @@ -896,6 +916,7 @@ private static uint HandleEvent(State state, ref StreamEvent evt) | |||
| return HandleEventPeerSendAborted(state, ref evt); | |||
| // Peer has stopped receiving data, don't send anymore. | |||
| case QUIC_STREAM_EVENT_TYPE.PEER_RECEIVE_ABORTED: | |||
| // CR: naming Receive | |||
| @@ -922,6 +943,7 @@ private static uint HandleEvent(State state, ref StreamEvent evt) | |||
| } | |||
| } | |||
|
|
|||
| // CR: Add high level how the reading works comment. | |||
Comment on lines
+1235
to
+1237
| // CR: canceled will be set when connection gets aborted, | ||
| // send with FIN followed in a quick succession by another send | ||
| // CR: we might ignore canceled, since there might be a race between connection getting closed and geting this event. |
There was a problem hiding this comment.
#59346
I'm not 100% sure I've got this right and correctly described it in the issue. @CarnaViire it's 14th bullet point in the issue. could you please check it there?
| uint status = MsQuicApi.Api.StreamReceiveCompleteDelegate(_state.Handle, (ulong)bufferLength); | ||
| QuicExceptionHelpers.ThrowIfFailed(status, "Could not complete receive call."); | ||
| } | ||
|
|
||
| // This can fail if the stream isn't started. | ||
| private long GetStreamId() | ||
| { | ||
| // CR: assert no lock is taken |
| @@ -1526,6 +1553,7 @@ private static uint HandleEventConnectionClose(State state) | |||
| private static Exception GetConnectionAbortedException(State state) => | |||
| ThrowHelper.GetConnectionAbortedException(state.ConnectionState.AbortErrorCode); | |||
|
|
|||
| // CR: naming ??? nobody knows a better name | |||
| @@ -1608,6 +1636,7 @@ private enum ReadState | |||
| Closed | |||
| } | |||
|
|
|||
| // CR: explain the state machine in comments | |||
| @@ -1616,6 +1645,7 @@ private enum ShutdownWriteState | |||
| ConnectionClosed | |||
| } | |||
|
|
|||
| // CR: explain the state machine in comments | |||
| @@ -1625,6 +1655,7 @@ private enum ShutdownState | |||
| ConnectionClosed | |||
| } | |||
|
|
|||
| // CR: explain the state machine in comments | |||
ManickaP
commented
Sep 20, 2021
| @@ -34,6 +34,8 @@ internal sealed class State | |||
|
|
|||
| public readonly SafeMsQuicConfigurationHandle? ConnectionConfiguration; | |||
| public readonly Channel<MsQuicConnection> AcceptConnectionQueue; | |||
| // CR: Connections before finished negotiation, we hold them back until they're fully connected / initialized. | |||
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Notes from yesterday.
cc @wfurt @CarnaViire