-
Notifications
You must be signed in to change notification settings - Fork 461
Fail in-flight invocations when worker channel shuts down #11159
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
base: dev
Are you sure you want to change the base?
Changes from all commits
99d2e23
78f3a99
d249c4a
eff32c2
ad650ed
16875db
31e0473
4b64ece
5ab7a94
dba3f31
b084a14
8c5cd2f
c904db4
04409f3
8ad875d
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 |
---|---|---|
@@ -0,0 +1,21 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
||
using System; | ||
|
||
namespace Microsoft.Azure.WebJobs.Script.Exceptions | ||
{ | ||
internal sealed class WorkerShutdownException : Exception | ||
{ | ||
public WorkerShutdownException() { } | ||
|
||
public WorkerShutdownException(string message) : base(message) { } | ||
|
||
satvu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
public WorkerShutdownException(string message, Exception innerException) : base(message, innerException) | ||
{ | ||
Reason = innerException?.Message ?? string.Empty; | ||
} | ||
|
||
public string Reason { get; set; } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -5,6 +5,8 @@ | |||||||||||||||||||||||
using System.Net.Http; | ||||||||||||||||||||||||
using System.Threading; | ||||||||||||||||||||||||
using System.Threading.Tasks; | ||||||||||||||||||||||||
using Microsoft.Azure.WebJobs.Script.Description; | ||||||||||||||||||||||||
using Microsoft.Azure.WebJobs.Script.Exceptions; | ||||||||||||||||||||||||
using Microsoft.Extensions.Logging; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
namespace Microsoft.Azure.WebJobs.Script.Http | ||||||||||||||||||||||||
|
@@ -30,11 +32,22 @@ public RetryProxyHandler(HttpMessageHandler innerHandler, ILogger logger) | |||||||||||||||||||||||
|
||||||||||||||||||||||||
protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
TaskCompletionSource<ScriptInvocationResult> resultSource = null; | ||||||||||||||||||||||||
if (request.Options.TryGetValue(ScriptConstants.HttpProxyScriptInvocationContext, out ScriptInvocationContext scriptInvocationContext)) | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
resultSource = scriptInvocationContext.ResultSource; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
var currentDelay = InitialDelay; | ||||||||||||||||||||||||
for (int attemptCount = 1; attemptCount <= MaxRetries; attemptCount++) | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
try | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
if (resultSource is not null && resultSource.Task.IsFaulted) | ||||||||||||||||||||||||
Comment on lines
42
to
+46
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. This check is performed on every retry attempt inside the loop. Consider moving this check outside the retry loop or caching the Task.IsFaulted result to avoid repeated property access.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback 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. This needs to be checked before every retry. |
||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
throw resultSource.Task.Exception?.InnerException ?? new InvalidOperationException("The function invocation tied to this HTTP request failed."); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
return await base.SendAsync(request, cancellationToken); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
catch (TaskCanceledException) when (cancellationToken.IsCancellationRequested) | ||||||||||||||||||||||||
|
@@ -51,6 +64,11 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage | |||||||||||||||||||||||
|
||||||||||||||||||||||||
currentDelay = Math.Min(currentDelay * 2, MaximumDelay); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
catch (WorkerShutdownException) | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
_logger.LogDebug("Language worker channel is shutting down. Request will not be retried."); | ||||||||||||||||||||||||
throw; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
catch (Exception ex) | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
var message = attemptCount == MaxRetries | ||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
||
using System.Collections.Generic; | ||
using System.Net.Http; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.AspNetCore.Http; | ||
using Microsoft.Azure.WebJobs.Script.Description; | ||
using Yarp.ReverseProxy.Forwarder; | ||
|
||
namespace Microsoft.Azure.WebJobs.Script.Http | ||
{ | ||
internal class ScriptInvocationRequestTransformer : HttpTransformer | ||
{ | ||
public override async ValueTask TransformRequestAsync(HttpContext httpContext, HttpRequestMessage proxyRequest, string destinationPrefix, CancellationToken cancellationToken) | ||
{ | ||
// this preserves previous behavior (which called the default transformer) - base method is also called inside of here | ||
await HttpTransformer.Default.TransformRequestAsync(httpContext, proxyRequest, destinationPrefix, cancellationToken); | ||
|
||
if (httpContext.Items.TryGetValue(ScriptConstants.HttpProxyScriptInvocationContext, out object result) | ||
&& result is ScriptInvocationContext scriptContext) | ||
{ | ||
proxyRequest.Options.TryAdd(ScriptConstants.HttpProxyScriptInvocationContext, scriptContext); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -206,7 +206,7 @@ public Task ShutdownAsync() | |||||
return Task.CompletedTask; | ||||||
} | ||||||
|
||||||
public Task<bool> RestartWorkerWithInvocationIdAsync(string invocationId) | ||||||
public Task<bool> RestartWorkerWithInvocationIdAsync(string invocationId, Exception exception = null) | ||||||
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. The new
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback 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. The unused parameter is here because this class implements |
||||||
{ | ||||||
// Since there's only one channel for httpworker | ||||||
DisposeAndRestartWorkerChannel(_httpWorkerChannel.Id); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
||
using System; | ||
satvu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
using System.Runtime.ExceptionServices; | ||
using System.Threading.Tasks; | ||
using Microsoft.AspNetCore.Hosting; | ||
using Microsoft.Azure.WebJobs.Host; | ||
using Microsoft.Azure.WebJobs.Script.WebHost; | ||
using Microsoft.Azure.WebJobs.Script.Workers; | ||
using Microsoft.Extensions.Logging; | ||
using Moq; | ||
using Xunit; | ||
|
||
namespace Microsoft.Azure.WebJobs.Script.Tests.Handlers | ||
{ | ||
public class WebScriptHostExceptionHandlerTests | ||
{ | ||
private readonly Mock<IApplicationLifetime> _mockApplicationLifetime; | ||
private readonly Mock<ILogger<WebScriptHostExceptionHandler>> _mockLogger; | ||
private readonly Mock<IFunctionInvocationDispatcherFactory> _mockDispatcherFactory; | ||
private readonly Mock<IFunctionInvocationDispatcher> _mockDispatcher; | ||
private readonly WebScriptHostExceptionHandler _exceptionHandler; | ||
|
||
public WebScriptHostExceptionHandlerTests() | ||
{ | ||
_mockApplicationLifetime = new Mock<IApplicationLifetime>(); | ||
_mockLogger = new Mock<ILogger<WebScriptHostExceptionHandler>>(); | ||
_mockDispatcherFactory = new Mock<IFunctionInvocationDispatcherFactory>(); | ||
_mockDispatcher = new Mock<IFunctionInvocationDispatcher>(); | ||
|
||
_mockDispatcherFactory.Setup(f => f.GetFunctionDispatcher()) | ||
.Returns(_mockDispatcher.Object); | ||
|
||
_exceptionHandler = new WebScriptHostExceptionHandler( | ||
_mockApplicationLifetime.Object, | ||
_mockLogger.Object, | ||
_mockDispatcherFactory.Object); | ||
} | ||
|
||
[Fact] | ||
public async Task OnTimeoutExceptionAsync_CallsRestartWorkerWithInvocationIdAsync_WithTimeoutException() | ||
{ | ||
var task = Task.CompletedTask; | ||
var timeoutException = new FunctionTimeoutException("Test timeout"); | ||
var exceptionInfo = ExceptionDispatchInfo.Capture(timeoutException); | ||
var timeoutGracePeriod = TimeSpan.FromSeconds(5); | ||
|
||
_mockDispatcher.Setup(d => d.State) | ||
.Returns(FunctionInvocationDispatcherState.Initialized); | ||
_mockDispatcher.Setup(d => d.RestartWorkerWithInvocationIdAsync(It.IsAny<string>(), It.IsAny<Exception>())) | ||
.Returns(Task.FromResult(true)); | ||
|
||
await _exceptionHandler.OnTimeoutExceptionAsync(exceptionInfo, timeoutGracePeriod); | ||
|
||
_mockDispatcher.Verify(d => d.RestartWorkerWithInvocationIdAsync( | ||
It.IsAny<string>(), | ||
timeoutException), Times.Once); | ||
} | ||
|
||
[Fact] | ||
public async Task OnTimeoutExceptionAsync_WhenTaskDoesNotCompleteWithinGracePeriod_RestartsWorker() | ||
{ | ||
// Arrange | ||
var invocationId = Guid.NewGuid(); | ||
var taskCompletionSource = new TaskCompletionSource<bool>(); | ||
var timeoutException = new FunctionTimeoutException("Test timeout"); | ||
var exceptionInfo = ExceptionDispatchInfo.Capture(timeoutException); | ||
var timeoutGracePeriod = TimeSpan.FromMilliseconds(100); // Short grace period | ||
|
||
_mockDispatcher.Setup(d => d.State) | ||
.Returns(FunctionInvocationDispatcherState.Initialized); | ||
_mockDispatcher.Setup(d => d.RestartWorkerWithInvocationIdAsync(It.IsAny<string>(), It.IsAny<Exception>())) | ||
.Returns(Task.FromResult(true)); | ||
|
||
// Don't complete the task to simulate it not finishing within the grace period | ||
|
||
// Act | ||
await _exceptionHandler.OnTimeoutExceptionAsync(exceptionInfo, timeoutGracePeriod); | ||
|
||
// Assert | ||
_mockDispatcher.Verify(d => d.RestartWorkerWithInvocationIdAsync( | ||
It.IsAny<string>(), | ||
timeoutException), Times.Once); | ||
} | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.