-
Notifications
You must be signed in to change notification settings - Fork 53
Add timeout to gRPC workitem streaming #390
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,11 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Text; | ||
| using DurableTask.Core; | ||
| using DurableTask.Core.Entities; | ||
| using DurableTask.Core.Entities.OperationFormat; | ||
| using DurableTask.Core.History; | ||
| using Grpc.Core; | ||
| using Microsoft.DurableTask.Entities; | ||
| using Microsoft.DurableTask.Worker.Shims; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
|
|
@@ -186,10 +184,18 @@ async ValueTask<OrchestrationRuntimeState> BuildRuntimeStateAsync( | |
|
|
||
| async Task ProcessWorkItemsAsync(AsyncServerStreamingCall<P.WorkItem> stream, CancellationToken cancellation) | ||
| { | ||
| // Create a new token source for timing out and a final token source that keys off of them both. | ||
| // The timeout token is used to detect when we are no longer getting any messages, including health checks. | ||
| // If this is the case, it signifies the connection has been dropped silently and we need to reconnect. | ||
| using var timeoutSource = new CancellationTokenSource(); | ||
| timeoutSource.CancelAfter(TimeSpan.FromSeconds(60)); | ||
| using var tokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellation, timeoutSource.Token); | ||
|
|
||
| while (!cancellation.IsCancellationRequested) | ||
| { | ||
| await foreach (P.WorkItem workItem in stream.ResponseStream.ReadAllAsync(cancellation)) | ||
| await foreach (P.WorkItem workItem in stream.ResponseStream.ReadAllAsync(tokenSource.Token)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this not throw if the connection is closed?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We thought it would too, but if you go into the |
||
| { | ||
| timeoutSource.CancelAfter(TimeSpan.FromSeconds(60)); | ||
| if (workItem.RequestCase == P.WorkItem.RequestOneofCase.OrchestratorRequest) | ||
| { | ||
| this.RunBackgroundTask( | ||
|
|
@@ -237,6 +243,20 @@ async Task ProcessWorkItemsAsync(AsyncServerStreamingCall<P.WorkItem> stream, Ca | |
| this.Logger.UnexpectedWorkItemType(workItem.RequestCase.ToString()); | ||
| } | ||
| } | ||
|
|
||
| if (tokenSource.IsCancellationRequested || tokenSource.Token.IsCancellationRequested) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I am missing something, but it seems unlikely this line would ever be
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above. We thought this behavior was an odd choice for the stream reader as well, but it's documented that it doesn't throw. |
||
| { | ||
| // The token has cancelled, this means either: | ||
| // 1. The broader 'cancellation' was triggered, return here to start a graceful shutdown. | ||
| // 2. The timeoutSource was triggered, return here to trigger a reconnect to the backend. | ||
halspang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (!cancellation.IsCancellationRequested) | ||
| { | ||
| // Since the cancellation came from the timeout, log a warning. | ||
| this.Logger.ConnectionTimeout(); | ||
| } | ||
|
|
||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.