[C#] Add cancellation token overloads to streaming interfaces by JamesNK · Pull Request #27886 · grpc/grpc

JamesNK · 2021-10-31T20:06:36Z

Adds default interface implementation with cancellation token overloads
Default implementation calls the current interface method. grpc-dotnet will override default implementation to use the cancellation token
Feature requires C# 8.0
Feature isn't supported on .NET Framework. Overloads will only be available with .NET Standard 2.1 target (i.e. .NET Core 3 or later)

JamesNK · 2021-10-31T22:10:21Z

Error related to updating SDK:

2021-10-31 14:27:53,001 START: run_tests_csharp_linux_dbg_native
2021-10-31 14:27:53,006 START: run_tests_csharp_linux_opt_native
2021-10-31 14:27:54,732 ++ dirname tools/run_tests/dockerize/build_docker_and_run_tests.sh
+ cd tools/run_tests/dockerize/../../..
++ pwd
+ git_root=/tmpfs/src/github/grpc
+ cd -
/tmpfs/src/github/grpc
++ basename tools/dockerfile/test/csharp_buster_x64
++ sha1sum tools/dockerfile/test/csharp_buster_x64/Dockerfile
++ cut -f1 '-d '
+ DOCKER_IMAGE_NAME=csharp_buster_x64:89ca6f76aece9e346f28331ebd6deda8fd5b1220
+ '[' grpctesting '!=' '' ']'
+ DOCKER_IMAGE_NAME=grpctesting/csharp_buster_x64:89ca6f76aece9e346f28331ebd6deda8fd5b1220
+ docker pull grpctesting/csharp_buster_x64:89ca6f76aece9e346f28331ebd6deda8fd5b1220
Error response from daemon: manifest for grpctesting/csharp_buster_x64:89ca6f76aece9e346f28331ebd6deda8fd5b1220 not found
Traceback (most recent call last):
  File "tools/run_tests/run_tests.py", line 1607, in <module>
    env=env)
  File "/usr/lib/python2.7/subprocess.py", line 541, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command 'tools/run_tests/dockerize/build_docker_and_run_tests.sh' returned non-zero exit status 1

jtattermusch · 2021-11-05T13:19:43Z

Unfortunately bumping the .NET SDK for Grpc.Core builds isn't that easy (more changes is needed in other places as well). I'd have to look into that separately.

jtattermusch · 2021-11-05T14:11:26Z

Note to self: Recent PR for upgrading the dotnet SDK: https://github.com/grpc/grpc/pull/26465/files

jtattermusch · 2021-11-09T12:36:32Z

I will look at this in detail once I successfully upgrade the dotnet SDK in the grpc/grpc repo: #27966 (it's proving difficult).

jtattermusch · 2021-11-17T09:43:39Z

SDK has been updated in #27966, you can rebase now.

JamesNK · 2022-01-17T23:25:43Z

ping @jtattermusch

jtattermusch

I wrote a few comments earlier, but forgot to hit "send", sorry for the delay.
See comments for concerns (To me it's unclear what the implementation would look like for Grpc.Core, and simply ignoring the cancellation token seems problematic).

Btw, in grpc/grpc-dotnet#1422 (comment) I pointed out that we are potentially missing a mechanism to cancel the entire call on the server side (on the client side cancelling a call is easy since you can pass cancellation token to a new call). One can "cancel" the server side call handler by simple throwing/returning from the handler function, but there is no other way.

Exposing overloads that take cancellationToken on individual write operations is basically just a proxy for cancelling the entire call, since the semantics of "cancel an individual write operation" is non-recoverable and the entire call will need to be cancelled anyway.

So perhaps we should first figure out the mechanism for cancelling the entire server side call (and expose it e.g. on ServerCallContext) after which adding the new WriteAsync overloads that take cancellationToken would become just syntactic sugar?

jtattermusch · 2021-11-29T12:56:57Z

+        /// </summary>
+        /// <param name="message">The message to be written. Cannot be null.</param>
+        /// <param name="cancellationToken">Cancellation token that can be used to cancel the operation.</param>
+        Task WriteAsync(T message, CancellationToken cancellationToken)


what is the intended behavior of the CancellationToken (if not CancellationToken.None) when on Grpc.Core and grpc-dotnet? I assume the plan is to trigger cancellation of the entire call if the cancellation token is triggered? (because there isn't much we can do otherwise). What is the plan for implementing that behavior in both Grpc.Core and grpc-dotnet?

Kestrel supports aborting in-progress requests. I haven't looked into exactly how it will work but it will either be:

The cancellation token is passed to the underlying writer and if the writer gets a cancellation then it will abort the request, or

WriteAsync subscribes to the cancellation token with a callback that calls HttpContext.Abort().

I don't know the internals of Grpc.Core.

Grpc.Core

Would Grpc.Core cancellation be supported with this code:

grpc/src/csharp/Grpc.Core/Internal/AsyncCallBase.cs

Lines 393 to 399 in 8410142

internal CancellationTokenRegistration RegisterCancellationCallbackForToken(CancellationToken cancellationToken)

{

if (cancellationToken.CanBeCanceled) return cancellationToken.Register(CancelCallFromToken, this);

return default(CancellationTokenRegistration);

}

private static readonly Action<object> CancelCallFromToken = state => ((AsyncCallBase<TWrite, TRead>)state).Cancel();

It is used by IAsyncStreamReader<T>.MoveNext which takes a cancellation token:

grpc/src/csharp/Grpc.Core/Internal/ServerRequestStream.cs

Lines 50 to 58 in 2d4f3c5

public async Task<bool> MoveNext(CancellationToken token)

{

using (call.RegisterCancellationCallbackForToken(token))

{

var result = await call.ReadMessageAsync().ConfigureAwait(false);

this.current = result;

return result != null;

}

}

It seems to me that what works for canceling while reading should also work for canceling while writing.

Grpc.Core

Actually, Grpc.Core only supports up to netstandard20 and net45. The new overload is netstandard21 or later so won't be available.

jtattermusch

LGTM.

jtattermusch · 2022-02-22T12:45:47Z

@apolcyn please add another LGTM.

jtattermusch · 2022-02-25T08:58:17Z

Added "RELEASE BLOCKER" to make sure it gets included in the upcoming 1.45.x branch cut.
The PR is ready to merge once @apolcyn approves.

The python and iOS binary size diff failures are unrelated (test flake and test infra timeout).

JamesNK requested review from apolcyn, jtattermusch and nicolasnoble as code owners October 31, 2021 21:23

JamesNK mentioned this pull request Oct 31, 2021

[C#] Add nullable type attributes to Grpc.Core.Api #27887

Merged

ctiller added the kokoro:force-run label Nov 4, 2021

grpc-kokoro removed the kokoro:force-run label Nov 4, 2021

grpc-checks Bot added the bloat/none label Nov 4, 2021

captainsafia approved these changes Nov 4, 2021

View reviewed changes

jtattermusch self-assigned this Nov 5, 2021

JamesNK force-pushed the jamesnk/streaming-cancellation branch from 5d6b5b3 to a0a5ee1 Compare November 17, 2021 17:18

JamesNK force-pushed the jamesnk/streaming-cancellation branch from a0a5ee1 to 886a8c8 Compare December 1, 2021 00:25

JamesNK mentioned this pull request Jan 24, 2022

Add IAsyncStreamWriter<T>.WriteAsync(T, CancellationToken) overload grpc/grpc-dotnet#1422

Closed

jtattermusch added release notes: yes Indicates if PR needs to be in release notes lang/C# labels Jan 28, 2022

jtattermusch reviewed Jan 28, 2022

View reviewed changes

Comment thread src/csharp/Grpc.Core.Api/IClientStreamWriter.cs Outdated

Add cancellation token overloads to streaming interfaces

d3971ec

JamesNK force-pushed the jamesnk/streaming-cancellation branch from 886a8c8 to d3971ec Compare February 8, 2022 21:32

PR feedback

af9f575

jtattermusch added the kokoro:force-run label Feb 22, 2022

grpc-kokoro removed the kokoro:force-run label Feb 22, 2022

jtattermusch approved these changes Feb 22, 2022

View reviewed changes

jtattermusch added the priority/P0/RELEASE BLOCKER label Feb 25, 2022

apolcyn approved these changes Mar 1, 2022

View reviewed changes

apolcyn merged commit 5ba6bef into grpc:master Mar 3, 2022

copybara-service Bot added the imported Specifies if the PR has been imported to the internal repository label Mar 4, 2022

	internal CancellationTokenRegistration RegisterCancellationCallbackForToken(CancellationToken cancellationToken)
	{
	if (cancellationToken.CanBeCanceled) return cancellationToken.Register(CancelCallFromToken, this);
	return default(CancellationTokenRegistration);
	}

	private static readonly Action<object> CancelCallFromToken = state => ((AsyncCallBase<TWrite, TRead>)state).Cancel();

	public async Task<bool> MoveNext(CancellationToken token)
	{
	using (call.RegisterCancellationCallbackForToken(token))
	{
	var result = await call.ReadMessageAsync().ConfigureAwait(false);
	this.current = result;
	return result != null;
	}
	}

Uh oh!

Conversation

JamesNK commented Oct 31, 2021

Uh oh!

JamesNK commented Oct 31, 2021

Uh oh!

jtattermusch commented Nov 5, 2021

Uh oh!

jtattermusch commented Nov 5, 2021

Uh oh!

jtattermusch commented Nov 9, 2021

Uh oh!

jtattermusch commented Nov 17, 2021

Uh oh!

JamesNK commented Jan 17, 2022

Uh oh!

jtattermusch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtattermusch Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

JamesNK Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

JamesNK Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jtattermusch left a comment

Choose a reason for hiding this comment

Uh oh!

jtattermusch commented Feb 22, 2022

Uh oh!

jtattermusch commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JamesNK Feb 8, 2022 •

edited

Loading

JamesNK Feb 8, 2022 •

edited

Loading

jtattermusch commented Feb 25, 2022 •

edited

Loading