Prevent the receive completion operation queued if unnecessary

[masa-koz]

Description

If the receive complete API is called on a user thread before QUIC_STREAM_EVENT_RECEIVE finished, MsQuic will call QuicStreamReceiveComplete immediately even though a completion operation is queued. And a completion operation will triger QuicStreamReceiveComplete even though RecvCompletionLength is zero.

So, this pull request introduces a new flag to track if the receive complete API was called inline and updates related logic in the MsQuic library. The most important changes include adding the new flag RecvCompletionInlineCalled, modifying the condition checks, and updating comments to reflect the new logic.

Testing

NA

Documentation

NA

[masa-koz]

@nibanks I modified this pr according to your suggestion.

[nibanks]

The more I think about this, the more I am wondering what the real problem is with the original behavior:

If the receive complete API is called on a user thread before QUIC_STREAM_EVENT_RECEIVE finished, MsQuic will call QuicStreamReceiveComplete immediately even though a completion operation is queued. And a completion operation will triger QuicStreamReceiveComplete even though RecvCompletionLength is zero.

What is the downside of calling the queued completion with a zero length?

[nibanks]

Is the problem that the zero drain results in a paused receive path?

[masa-koz]

Yes. I'm afraid that I forgot the detail, but recv path paused even though the data arrived.

[nibanks]

Yes. I'm afraid that I forgot the detail, but recv path paused even though the data arrived.

Thanks. Let me think on this problem a bit more to see what might be the simplest way forward.

[nibanks]

@guhetier can you please add this PR to your to-do list to help drive to completion? We need to get this fixed before the next release.

[guhetier]

Hi @masa-koz, are you still interested in working on this PR?
If yes, I'm happy to help you get to a lock-free solution, otherwise, I can take over to fix this issue.

After looking at the problem again, I think we can avoid the lock by storing both ReceiveCallActive and RecvCompletionLength in the same uint64_t and updating them in one atomic operation:

1. The top bit of RecvCompletionLength is used for ReceiveCallActive
2. In QuicStreamRecvFlush, Stream->Flags.ReceiveCallActive = TRUE; can become something like:

    InterlockedExchangeAdd64((int64_t*)&Stream->RecvCompletionLength, 0x8000000000000000);

3. ... and after the notification returns, we can both get the number of bytes to complete and clear the flag with:

    BufferLength = InterlockedExchangeAdd64((int64_t*)&Stream->RecvCompletionLength, -0x8000000000000000);
    BufferLength &= ~0x8000000000000000;
    InterlockedExchangeAdd64((int64_t*)&Stream->RecvCompletionLength, BufferLength);

4. In MsQuicStreamReceiveComplete, we start by always using InterlockedExchangeAdd64 to add the number of completed bytes to Stream->RecvCompletionLength, and we check the return value top bit to know if a receive call is pending, or if we need to queue a work item.

I think this should work. There is still a possibility of a race in "Multiple Receive" mode, but in that mode, a receive completion work item with 0 bytes completed is not a problem, we don't stop indicating receives.

This is similar to what Nick suggested in a comment, but slightly simpler.

[masa-koz]

I'm still interested. But I have no time now. Could you mind waiting for a few days?

[guhetier]

It can wait a few days, we want to make sure we have this done for the next release (which doesn't have a specific date yet). Thanks for your contributions!

[nibanks]

There are also merge conflicts preventing the CI from being run.

[codecov]

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.

Project coverage is 86.95%. Comparing base (186accc) to head (dabf3e5).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
src/core/api.c	50.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4900      +/-   ##
==========================================
+ Coverage   86.28%   86.95%   +0.67%     
==========================================
  Files          59       59              
  Lines       18011    18013       +2     
==========================================
+ Hits        15541    15664     +123     
+ Misses       2470     2349     -121     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[nibanks]

Here's a slight redesign that makes it cleaner and more efficient (I think).

[masa-koz]

@nibanks
I've changed the code to follow your suggestion.

[nibanks]

Could you replace merge/rebase main to get a few recent CI fixes? Then I think we'll be good to go. Thanks so much for working with us on this PR!

[guhetier]

I am not convinced this is a good way to handle the error...
If we simply goto Exit with a log, we silently break our contract with the app: the completed some bytes but we won't restart receive indications (or in multi-receive mode, they are now out of sync).

IMO, this should be a fatal error for the connection.

@nibanks, opinions?

(I thought I commented on this earlier but can't find it anymore...)

[nibanks]

For one, you have to be careful about aborting the connection on the app thread here. I think we may do something special above in one of the other functions. But you're probably right that we should abort the connection if this happens.

[nibanks]

See this code in StreamSend above:

            //
            // We can't fail the send at this point, because we're already queued
            // the send above. So instead, we're just going to abort the whole
            // connection.
            //
            if (InterlockedCompareExchange16(
                    (short*)&Connection->BackUpOperUsed, 1, 0) != 0) {
                goto Exit; // It's already started the shutdown.
            }
            Oper = &Connection->BackUpOper;
            Oper->FreeAfterProcess = FALSE;
            Oper->Type = QUIC_OPER_TYPE_API_CALL;
            Oper->API_CALL.Context = &Connection->BackupApiContext;
            Oper->API_CALL.Context->Type = QUIC_API_TYPE_CONN_SHUTDOWN;
            Oper->API_CALL.Context->CONN_SHUTDOWN.Flags = QUIC_CONNECTION_SHUTDOWN_FLAG_SILENT;
            Oper->API_CALL.Context->CONN_SHUTDOWN.ErrorCode = (QUIC_VAR_INT)QUIC_STATUS_OUT_OF_MEMORY;
            Oper->API_CALL.Context->CONN_SHUTDOWN.RegistrationShutdown = FALSE;
            Oper->API_CALL.Context->CONN_SHUTDOWN.TransportShutdown = TRUE;
            QuicConnQueueHighestPriorityOper(Connection, Oper);