-
Notifications
You must be signed in to change notification settings - Fork 263
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
Adding Timeout Property to DurableHttpRequest #1547
Conversation
|
||
return JsonConvert.SerializeObject(durableHttpResponse); | ||
} | ||
catch (OperationCanceledException ex) // when (cts.Token.IsCancellationRequested) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this more accurate, it could be catch (OperationCanceledException ex) when (cts.Token.IsCancellationRequested)
(added the commented out part). This way we know that the exception came when a cancel request was sent to CancellationTokenSource
. The only issue with this is adding tests that mirror the action of cancelling a CancellationTokenSource
. The Mock<HttpMessageHandler>
Setup call takes ItExpr.IsAny<CancellationToken>()
and the test would need the CancellationTokenSource
to cancel the task, not the CancellationToken
.
I saw that that ItExpr.IsAny()
could evaluate conditions, but didn't see anything about sending in another object to manipulate. Let me know if there's a way to pass in a CancellationTokenSource using Moq.
This is how the HttpMessageHandler
is mocked right now:
var handlerMock = new Mock<HttpMessageHandler>(MockBehavior.Strict);
handlerMock
.Protected()
.Setup<Task<HttpResponseMessage>>("SendAsync", ItExpr.IsAny<HttpRequestMessage>(), ItExpr.IsAny<CancellationToken>())
.ReturnsAsync(httpResponseMessage);
src/WebJobs.Extensions.DurableTask/Listener/TaskHttpActivityShim.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answered some questions and gave some feedback, but there is still some stuff that needs to be clarified.
src/WebJobs.Extensions.DurableTask/ContextImplementations/DurableOrchestrationContext.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/Listener/TaskHttpActivityShim.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/Listener/TaskHttpActivityShim.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/Listener/TaskHttpActivityShim.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/Listener/TaskHttpActivityShim.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a bit of my own feedback here.
src/WebJobs.Extensions.DurableTask/ContextImplementations/DurableOrchestrationContext.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/ContextImplementations/DurableOrchestrationContext.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/Listener/TaskHttpActivityShim.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/Listener/TaskHttpActivityShim.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/Listener/TaskHttpActivityShim.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/Listener/TaskHttpActivityShim.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/ContextImplementations/DurableOrchestrationContext.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I have a small nit, but not a blocker. It's up to you whether you want to fix it before merging.
src/WebJobs.Extensions.DurableTask/Listener/TaskHttpActivityShim.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/Listener/TaskHttpActivityShim.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resetting my approval status. No blockers from me.
These changes resolve #1278 by adding a
Timeout
property toDurableHttpRequest
. This property defines the timeout for each individual HTTP request. This timeout applies to each asynchronous polling request too.