Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[QUIC] Adding CompleteWritesAsync #104032

Closed
wants to merge 1 commit into from

Conversation

ManickaP
Copy link
Member

Implements async CompleteWritesAsync which will eventually replace sync CompleteWrites.

The behavior in this PR now mimics combination of CompleteWrites and WritesClosed. This PR also changes behavior of WritesAsync(..., completeWrites: true) to also wait for SEND_SHUTDOWN_COMPLETE (or abort). This PR brings few breaking behavioral changes:

  • WritesAsync(..., completeWrites: true)
    • it's now not possible to separate when the data are buffered (SEND_COMPLETE) and when the completion is ACKed, making the "last write" potentially much longer
    • the "last write" now may start throwing in case of local Abort(write) (RESET_STREAM) or remote Abort(read) (STOP_SENDING)
  • CompleteWritesAsync
    • as it's now async, the user doesn't have an easy way to fire-and-forget stream write completion

The behavior is up for discussion, some options:

  • don't do anything and leave the QuicStream as it was before this change
  • only report success in CompletWritesAsync and never throw (Exception is still part of CompleteWrites task)

Unfortunately, this effectively reverts #103902 as the WriteAsync now have to chain to async calls in a row...

Fixes #103434

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@MihaZupan
Copy link
Member

Unfortunately, this effectively reverts #103902 as the WriteAsync now have to chain to async calls in a row...

But only for the one final write that has completeWrites set, right?
Could we avoid that by changing

await valueTask.ConfigureAwait(false);
if (completeWrites)
{
    await _sendTcs.GetFinalTask(this).ConfigureAwait(false);
}

to

return completeWrites
    ? WaitForTaskAndSendTcsAsync(valueTask)
    : valueTask;

async WaitForTaskAndSendTcsAsync(ValueTask valueTask)
{
    await valueTask.ConfigureAwait(false);
    await _sendTcs.GetFinalTask(this).ConfigureAwait(false);
}

?

We use similar tricks elsewhere in HTTP, e.g. in HttpConnection.WriteAsync.


Equivalent to using WriteAsync with completeWrites: true.

Are we exposing the CompleteWritesAsync method purely as a nicer-to-write helper compared to WriteAsync([], true)?

@ManickaP
Copy link
Member Author

But only for the one final write that has completeWrites set, right?

Yes and the trick would work 👍


Are we exposing the CompleteWritesAsync method purely as a nicer-to-write helper compared to WriteAsync([], true)?

Basically yes.

@rzikm rzikm changed the title [QUIC] Adding CompletWritesAsync [QUIC] Adding CompleteWritesAsync Jun 27, 2024
@ManickaP
Copy link
Member Author

ManickaP commented Jul 2, 2024

Closing, decided not to go with the API.

@ManickaP ManickaP closed this Jul 2, 2024
@ManickaP ManickaP deleted the quic-complete-writes-async branch July 2, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: [QUIC] Make CompleteWritesAsync
2 participants