feat: timeout http calls and add async headers calls #516

viacheslav-rostovtsev · 2021-12-08T02:27:52Z

No description provided.

jskeet

I'm fine with the general approach... I had vague memories of having helper methods to check for cancellation like this, but I'm not sure whether we actually do.

Google.Api.Gax.Grpc/Rest/RestChannel.cs

jskeet · 2021-12-13T09:34:16Z

Google.Api.Gax.Grpc/Rest/RestChannel.cs

@@ -60,9 +62,12 @@ public RestChannel(RestServiceCollection serviceCollection, string endpoint, Cha
            var restMethod = _serviceCollection.GetRestMethod(method);

            var cancellationTokenSource = new CancellationTokenSource();
+            if (options.Deadline.HasValue)
+            {
+                cancellationTokenSource = new CancellationTokenSource(options.Deadline.Value - DateTime.UtcNow);


Please add a TODO here to inject an IClock in the future. That doesn't need to block GA though.

(We should probably consider what happens if the deadline has already passed, too. If we end up with a TimeSpan with TotalMilliseconds of -1, that ends up as an infinite deadline instead of one just passed. I think we can fix that shortly after GA though.)

Not an infinite deadline, but yet another exception of a different type. Fixed.

jskeet · 2021-12-13T09:36:18Z

Google.Api.Gax.Grpc/Rest/RestChannel.cs

+            var delayTask = Task.Delay(-1, cancellationToken);
+            if (delayTask == await Task.WhenAny(addAuthHeadersTask, delayTask).ConfigureAwait(false))
+            {
+                throw new InvalidOperationException("Timeout was reached when waiting for auth headers to be added for a method {restMethod.FullName}.");


Throw OperationCanceledException instead of InvalidOperationException?

I changed to the TimeoutException.

jskeet · 2021-12-13T09:36:43Z

Google.Api.Gax.Grpc/Rest/RestChannel.cs

+                httpResponseMessage = await _httpClient.SendAsync(httpRequest,
+                    HttpCompletionOption.ResponseContentRead, cancellationToken).ConfigureAwait(false);
+            }
+            catch(TaskCanceledException ex)


I think we can remove this catch block - just let the cancellation propagate.

The TaskCancelledException is extremely uninformative in Core 2.1 -- does not have an inner exception or any reference to a timeout. (At later versions I think they made it wrap a TimeoutException).
For the consistency I think wrapping it into a TimeoutException would be best.

Tiny nit: space after catch.

But more importantly, I'm not at all sure about all the uses of TimeoutException here.

I've just tried making an RPC via gRPC, and it failed with a DeadlineExceeded RpcException. I don't know whether that came from the server or whether it was generated locally... would it be reasonable to synthesize that here? (It's easy enough to do.)

@amanda-tarafa any thoughts.

There's nowhere else where we use TimeoutException that I recall, so I don't think this would be consitent with anything we already have. In Apiary, we just buble up the cancellation exception regardless of whether it's because of a timeout or something else.

I think here I'd like best to be consistent with the rest of error mapping we are doing, so +1 to throwing an RpcException with a DeadlineExceeded status.

Wether we throw inmediately after we know that the deadline is exceeded, or whether we fake a 503 response, is another thing. I' would lean towards throwing inmediately, but then I think we should modify the GetStatus method (line 145) to take into account the case in which the task is faulted with an RpcException, so we can return the Status contained by that RpcException itself.

We use TimeoutException in Polling, but only there.

I thought we had cunning code in RetryAttempt to avoid sleeping if we know that by the time we've finished sleeping the deadline will have passed, but I can't find that now.

Hmm. We do have a slight issue here - while it's tempting to throw away the TaskCanceledException, if this is actually because the caller passed in a CancellationToken and that got canceled, it's weird to change that.

What we could potentially do is use the deadline and current time to work out what to throw - that's pretty weird, but would probably give the right expected semantics.

@jskeet The cunning code is here
But what I meant was just to throwing vs. faking a 503 response, not throwing inmediately vs. delay throwing, which is why I think you brought the cunning code up?

@viacheslav-rostovtsev You can include an inner exception on the RpcException via Status.DebugException, we are already doing so on GetStatus method (line 145).

There is no caller, only us. We generate a token from a source we control. The httpClient.SendAsync docs say that TaskCancelled exception happens because The request failed due to timeout.
I would throw an RpcException here, then we can ask the Grpc team to give us a constructor that takes an InnerException, and update the code to wrap a TaskCancelled later.
(actually, I would just throw a TimeoutException, because I think that throwing a GRPC-specific exception in a non-GRPC context creates more confusion than it solves, but I can see the argument for it)

I'd argue that this is a gRPC context, in terms of the abstraction GAX exposes - we're implementing a gRPC CallInvoker. So it makes sense to talk in gRPC concepts - just like we translate HTTP status codes into gRPC ones.

Even though we generate one CancellationToken, there's another which may be present in the CallOptions... it looks like we're not observing that at the moment, which also needs to be fixed. (The TODO was meant to cover all aspects of CallOptions; a deadline is only part of that.) But we can probably differentiate between the cancellation token we've created (just for the deadline) and one that has been provided by the caller. We can check whether our deadline-oriented cancellation token has been cancelled, and translate that into an RpcException, but leave anything else.

Updated to account for this, please TAL

amanda-tarafa · 2021-12-14T09:45:43Z

Google.Api.Gax.Grpc/Rest/RestChannel.cs

+                var delayInterval = options.Deadline.Value - DateTime.UtcNow;
+                if (delayInterval.TotalMilliseconds <= 0)
+                {
+                    throw new TimeoutException($"The timeout was reached when calling a method `{restMethod.FullName}`");


This to me reads more like an ArgumentException, the conditions under which this exception is thrown are not recoverable, if this exception is thrown is very likely that there's a bug in the calling code and we should signal as such.

No, it shouldn't be an ArgumentException - it's just a deadline which we happen to have exceeded already. It might not have been exceeded when the method was called, just a really, really short deadline.

Hmmm, if we were going to be that strict, then we should continue to inspect the deadline even after the HTTP request returned succesfully, i.e. while we parse the response, etc.
I think a deadline so small that we cannot even do prep for the part of the process that takes the significant amount of time could be considered a bug, and different from a "really small deadline" that some times will not be enough to complete the time consuming part of the process. And I think we could signal that differently.

But I'm happy to disagree on this.

It's not a matter of being strict as such - we're only throwing something because we know the deadline has already passed before we make the RPC; this is only a check to make sure we don't pass an invalid argument to Task.Delay.

amanda-tarafa · 2021-12-14T09:51:24Z

Google.Api.Gax.Grpc/Rest/RestChannel.cs

+            var addAuthHeadersTask = AddAuthHeadersAsync(httpRequest, restMethod);
+            var delayTask = Task.Delay(-1, cancellationToken);
+            if (delayTask == await Task.WhenAny(addAuthHeadersTask, delayTask).ConfigureAwait(false))
+            {
+                throw new TimeoutException($"Timeout was reached when waiting for auth headers to be added for a method `{restMethod.FullName}`.");
+            }


I would add this logic on AddAuthHeadersAsync passing the cancellation token as a parameter. It's all internal anyway and then this ends up cleaner.

amanda-tarafa · 2021-12-14T10:54:26Z

Google.Api.Gax.Grpc/Rest/RestChannel.cs

+                httpResponseMessage = await _httpClient.SendAsync(httpRequest,
+                    HttpCompletionOption.ResponseContentRead, cancellationToken).ConfigureAwait(false);
+            }
+            catch(TaskCanceledException ex)


There's nowhere else where we use TimeoutException that I recall, so I don't think this would be consitent with anything we already have. In Apiary, we just buble up the cancellation exception regardless of whether it's because of a timeout or something else.

I think here I'd like best to be consistent with the rest of error mapping we are doing, so +1 to throwing an RpcException with a DeadlineExceeded status.

Wether we throw inmediately after we know that the deadline is exceeded, or whether we fake a 503 response, is another thing. I' would lean towards throwing inmediately, but then I think we should modify the GetStatus method (line 145) to take into account the case in which the task is faulted with an RpcException, so we can return the Status contained by that RpcException itself.

amanda-tarafa

I left these comments earlier but forgot to submit the review...

amanda-tarafa · 2021-12-14T12:46:02Z

Google.Api.Gax.Grpc/Rest/RestChannel.cs

+                httpResponseMessage = await _httpClient.SendAsync(httpRequest,
+                    HttpCompletionOption.ResponseContentRead, cancellationToken).ConfigureAwait(false);
+            }
+            catch(TaskCanceledException ex)


@jskeet The cunning code is here
But what I meant was just to throwing vs. faking a 503 response, not throwing inmediately vs. delay throwing, which is why I think you brought the cunning code up?

@viacheslav-rostovtsev You can include an inner exception on the RpcException via Status.DebugException, we are already doing so on GetStatus method (line 145).

amanda-tarafa · 2021-12-14T13:53:20Z

Google.Api.Gax.Grpc/Rest/RestChannel.cs

+                var delayInterval = options.Deadline.Value - DateTime.UtcNow;
+                if (delayInterval.TotalMilliseconds <= 0)
+                {
+                    throw new TimeoutException($"The timeout was reached when calling a method `{restMethod.FullName}`");


Hmmm, if we were going to be that strict, then we should continue to inspect the deadline even after the HTTP request returned succesfully, i.e. while we parse the response, etc.
I think a deadline so small that we cannot even do prep for the part of the process that takes the significant amount of time could be considered a bug, and different from a "really small deadline" that some times will not be enough to complete the time consuming part of the process. And I think we could signal that differently.

But I'm happy to disagree on this.

jskeet

Okay, I think I'm now happy barring one nit. Would like approval from @amanda-tarafa as well though.

jskeet · 2021-12-14T14:03:57Z

Google.Api.Gax.Grpc/Rest/RestChannel.cs

+                {
+                    httpResponseMessage = await _httpClient.SendAsync(httpRequest, HttpCompletionOption.ResponseContentRead, linkedCts.Token).ConfigureAwait(false);
+                }
+                catch(TaskCanceledException)


Nit: space after catch. But actually, we can simplify this anyway:

catch (TaskCanceledException) when (deadline.IsCancellationRequested) { throw new RpcException(new Status(StatusCode.DeadlineExceeded, $"The timeout was reached when calling a method `{restMethod.FullName}`")); }

No need to rethrow explicitly.

amanda-tarafa · 2021-12-14T14:27:44Z

Google.Api.Gax.Grpc/Rest/RestChannel.cs

+                throw new RpcException(new Status(StatusCode.DeadlineExceeded, $"Timeout was reached when waiting for auth headers to be added for a method `{restMethod.FullName}`."));
+            }
+
+            if (clientCancellationTask == resultTask)


channelTask may have failed as well, in which case we are not failing, we are just not adding any headers, I guess we should buble the channelTask failure if there was any?

I wonder, why don't you pass the linked token as a parameter here and then do something like:

if (resultTask.IsFaulted)
{
throw resultTask.Exception;
}

And then you catch that the same way you catch the one in the calling _httpClient.SendAsync(...). If you want to keep the different messages you can set a flag after you have added the headers to diferentiate which operation threw.

I've implemented a version of this.

jskeet

This is looking good to me - but Amanda has been better at spotting problems than I have on this one, so I'll defer to her. Nearly there!

jskeet · 2021-12-14T21:07:05Z

Google.Api.Gax.Grpc/Rest/RestChannel.cs

+            {
+                if (channelTask.Exception is object)
+                {
+                    throw channelTask.Exception;


I think we'd want ExceptionDispatchInfo.Capture(channelTask.Exception).Throw(); here, to preserve the stack trace.

amanda-tarafa · 2021-12-15T10:22:14Z

Google.Api.Gax.Grpc/Rest/RestChannel.cs

+                    throw channelTask.Exception;
+                }
+
+                throw new InvalidOperationException($"An unknown error has occurred when adding auth headers to a method `{restMethod.FullName}`.");


Does this happen? Can the task be faulted and the Exception be null? I would have though not.

I don't think so.

The docs agree. Fixed.

amanda-tarafa · 2021-12-15T10:23:16Z

Google.Api.Gax.Grpc/Rest/RestChannel.cs

+            if (combinedCancellationTask == resultTask)
+            {
+                throw new TaskCanceledException(
+                    $"The task was cancelled by the caller while waiting for the auth headers to be added for a method `{restMethod.FullName}`.");


nit: By the caller, or because we hit the deadline, right?

If we hit a deadline, we'll wrap this in the exception that says that the timeout was reached. If we did not hit the deadline, there is no need for this or.

amanda-tarafa · 2021-12-15T10:28:16Z

Google.Api.Gax.Grpc/Rest/RestChannel.cs

+            var combinedCancellationTask = Task.Delay(-1, combinedCancellationToken);
+            var channelTask = _channelAuthInterceptor(context, metadata);
+            var resultTask = await Task.WhenAny(channelTask, combinedCancellationTask).ConfigureAwait(false);
+            if (combinedCancellationTask == resultTask)


I think that if this is the case, then resultTask will be faulted and resultTask.Exception will contain a TaskCanceledException. The only possible state in which combinedCancellationTask may run to completion is faulted.

Or am I missing something?

My point is that I think lines from 135 to 149 may be substituted by:

if(resultTask.IsFaulted) { ExceptionDispatchInfo.Capture(resultTask.Exception).Throw(); }

Or what am I missing?

No, I think you're right. (I thought before that we'd want to provide a different exception on timeout for auth headers, but in this case I think it's fine for the task cancellation to just propagate.)

The Task.Delay(-1, combinedCancellationToken) does not get faulted and does not have an exception when the cancellation gets called on the token. It gets the status: Cancelled and that's it.

Ah, then you can change lines from 135 to 149 with:

await resultTask;

This will throw the TaskCancelledException if cancellation happened, or whatever exception happened if the real task failed.
If you get past this line of code, then there was no error or cancellation.

I think that will be cleaner and easier to understand than all the code we have now.

Yes, this makes sense. Updated.

amanda-tarafa

LGTM! Thanks, it looks really way simpler now.

jskeet

Great - I'll squash and merge.

viacheslav-rostovtsev requested a review from a team as a code owner December 8, 2021 02:27

jskeet reviewed Dec 8, 2021

View reviewed changes

Google.Api.Gax.Grpc/Rest/RestChannel.cs Outdated Show resolved Hide resolved

Google.Api.Gax.Grpc/Rest/RestChannel.cs Outdated Show resolved Hide resolved

Google.Api.Gax.Grpc/Rest/RestChannel.cs Outdated Show resolved Hide resolved

viacheslav-rostovtsev marked this pull request as draft December 9, 2021 01:03

viacheslav-rostovtsev marked this pull request as ready for review December 9, 2021 23:21

viacheslav-rostovtsev requested a review from jskeet December 9, 2021 23:21

jskeet reviewed Dec 13, 2021

View reviewed changes

viacheslav-rostovtsev requested a review from jskeet December 13, 2021 21:53

amanda-tarafa reviewed Dec 14, 2021

View reviewed changes

viacheslav-rostovtsev added 9 commits December 14, 2021 05:18

feat: timeout http calls and add async headers calls

97a2f99

working version

62fbe59

remove testing artefacts

22103b0

the only stable one

1579dfc

add cancellation to adding headers task

04c07e3

restore

845da9c

fix message

77e5936

messages and exceptions

816bc06

with calloptions cancellation

8c63f20

viacheslav-rostovtsev force-pushed the feat/timeout_http_calls branch from 9cbac8b to 8c63f20 Compare December 14, 2021 13:18

fixup

78e3d8e

amanda-tarafa reviewed Dec 14, 2021

View reviewed changes

jskeet approved these changes Dec 14, 2021

View reviewed changes

amanda-tarafa reviewed Dec 14, 2021

View reviewed changes

latest PR changes

4abeefc

jskeet approved these changes Dec 14, 2021

View reviewed changes

amanda-tarafa reviewed Dec 15, 2021

View reviewed changes

viacheslav-rostovtsev added 3 commits December 16, 2021 13:07

fix PR comments

fed162c

simplify the task result handling

04de915

fixup

b2abd85

amanda-tarafa approved these changes Jan 5, 2022

View reviewed changes

jskeet approved these changes Jan 5, 2022

View reviewed changes

jskeet merged commit afb1dbf into googleapis:main Jan 5, 2022

release-please bot mentioned this pull request Dec 8, 2022

chore(main): release Google.Api.Gax 2.3.0-beta01 #665

Closed

feat: timeout http calls and add async headers calls #516

feat: timeout http calls and add async headers calls #516

Conversation

viacheslav-rostovtsev commented Dec 8, 2021

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viacheslav-rostovtsev Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viacheslav-rostovtsev Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

viacheslav-rostovtsev Dec 14, 2021 •

edited

Loading

viacheslav-rostovtsev Dec 14, 2021 •

edited

Loading