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] Should QuicStream cancellation abort? #72607

Closed
ManickaP opened this issue Jul 21, 2022 · 5 comments
Closed

[QUIC] Should QuicStream cancellation abort? #72607

ManickaP opened this issue Jul 21, 2022 · 5 comments
Assignees
Milestone

Comments

@ManickaP
Copy link
Member

The idea behind #55485 was to either have soft cancellation - you can still use the stream afterwards; or abort on cancellation - what we do now.
If you want to abort the stream with another code, you can abort it on your own. You wouldn't pass in your cancellation token to read/write but rather register for it on your own and call Abort(Read/Write) which will unblock the pending operation.

Neither solution is perfect, the question is which one is better for most of the users:

  • aborting with default code, where some users like you will need to handle cancellation on their own
  • allowing subsequent read/writes with potential to take unreasonable long since we don't have a way to cancel pending read/write in the underlying library I'm open to this, we can certainly do it from the technical point of view.

What we had in .NET until now, not aborting but also not allowing any subsequent operation, seems like the least expected behavior, so I'd rather not bring it back.

Originally posted by @ManickaP in #69675 (comment)

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 21, 2022
@ManickaP
Copy link
Member Author

Use case from @bentoi from #69675 (comment)

I'm not using the Quic API yet but I would like to use it once it's released with .NET Core 7.0. I'm planning to use it for an application protocol similar to HTTP/3. For now, I'm using a TCP based transport similar to HTTP/2. Cancellation of a request aborts the underlying stream with a specific error code (like Http3ErrorCode.RequestCancelled). A request can also be canceled on connection closure and in this case I'd like to send another error code.

So basically, right now I'm using something like the following:

var linkedSource = CancellationTokenSource.CreateLinkedTokenSource(cancel, _closeCancellationSource.Token);
try
{
   stream.ReadAsync(linkedSource.Token);
}
catch (OperationCanceledException) when (_closeCancellationSource.IsCancellationRequested)
{
   stream.Abort(QuicAbortDirection.Read, ConnectionClosedErrorCode);
   throw new ConnectionClosedException();
}
catch (OperationCanceledException) when (cancel.IsCancellationRequested)
{
   stream.AbortRead(RequestCanceledErrorCode);
   throw;
}

I can re-arrange the code to instead do something like the following:

using _ = _closeCancellationSource.Token.Register(
     () => stream.Abort(QuicAbortDirection.Read, ConnectionClosedErrorCode));
try
{
     stream.ReadAsync(cancel);
}
catch (QuicException) when (_closeCancellationSource.IsCancellationRequested)
{
    // The stream was probably aborted on connection closure
    throw new ConnectionClosedException(); 
}
catch (OperationCanceledException)
{
    // request was canceled and stream aborted by the Quic implementation with default stream error code
    throw;
}

It's not more complicated so I'll use this instead.

However, I find that using the same default error code for different purposes isn't right. That's why I brought this up and questioned whether or not soft cancellation was a better approach. But I see your point about implicitly aborting the stream on cancellation.

@ghost
Copy link

ghost commented Jul 21, 2022

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

Issue Details

The idea behind #55485 was to either have soft cancellation - you can still use the stream afterwards; or abort on cancellation - what we do now.
If you want to abort the stream with another code, you can abort it on your own. You wouldn't pass in your cancellation token to read/write but rather register for it on your own and call Abort(Read/Write) which will unblock the pending operation.

Neither solution is perfect, the question is which one is better for most of the users:

  • aborting with default code, where some users like you will need to handle cancellation on their own
  • allowing subsequent read/writes with potential to take unreasonable long since we don't have a way to cancel pending read/write in the underlying library I'm open to this, we can certainly do it from the technical point of view.

What we had in .NET until now, not aborting but also not allowing any subsequent operation, seems like the least expected behavior, so I'd rather not bring it back.

Originally posted by @ManickaP in #69675 (comment)

Author: ManickaP
Assignees: -
Labels:

untriaged, area-System.Net.Quic

Milestone: -

@ManickaP ManickaP added this to the 8.0.0 milestone Jul 21, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jul 21, 2022
@ManickaP
Copy link
Member Author

Triage: we should look at what sockets and ssl stream do in these cases and do the same thing. We should decide in 8.0 timeframe.

@ManickaP
Copy link
Member Author

ManickaP commented Aug 9, 2023

I'm closing this as the decision is to keep it as it it.
Major technical reason being that we've closed #73691 as not viable at the moment, which we would need to do "soft cancellation" for write operation.

@ManickaP ManickaP closed this as not planned Won't fix, can't repro, duplicate, stale Aug 9, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant