-
Notifications
You must be signed in to change notification settings - Fork 357
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
Provide a hook for invoke #1167
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 |
---|---|---|
@@ -0,0 +1,26 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
|
||
namespace Microsoft.Azure.WebJobs.Host.Executors | ||
{ | ||
/// <summary> | ||
/// Interface defining the contract for executing a triggered function. | ||
/// Allows a hook around the underlying execution. | ||
/// This should only be used by extensions that need very specific control over the invocation. | ||
/// </summary> | ||
public interface ITriggeredFunctionExecutorWithHook | ||
{ | ||
/// <summary> | ||
/// Try to invoke the triggered function using the values specified. | ||
/// </summary> | ||
/// <param name="input">The trigger invocation details.</param> | ||
/// <param name="cancellationToken">The cancellation token</param> | ||
/// <param name="hook">a hook that wraps the underlying invocation</param> | ||
/// <returns>A <see cref="FunctionResult"/> describing the results of the invocation.</returns> | ||
Task<FunctionResult> TryExecuteAsync(TriggeredFunctionData input, CancellationToken cancellationToken, Func<Func<Task>, Task> hook); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,14 +2,15 @@ | |
// Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.Azure.WebJobs.Host.Protocols; | ||
using Microsoft.Azure.WebJobs.Host.Triggers; | ||
|
||
namespace Microsoft.Azure.WebJobs.Host.Executors | ||
{ | ||
internal class TriggeredFunctionExecutor<TTriggerValue> : ITriggeredFunctionExecutor | ||
internal class TriggeredFunctionExecutor<TTriggerValue> : ITriggeredFunctionExecutor, ITriggeredFunctionExecutorWithHook | ||
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. I think we should find a more general solution to this, rather than hack in this hook. Let's give it some thought. E.g. @fabiocav had some ideas about introducing an Owin like middelware model to allow people to plug in at various places in our pipeline. If we don't go that far, perhaps an interface people can register in our services collection that we call at various points in our pipeline for extensibility. Even the work we're doing now for Invoke Filters applies to this scenario if we were to allow filters to be registered host level. 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. I'm very open to alternatives. The criteria for me was the simplest solution that 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. An alternative to consider would be to extend the existing 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. I considered that, but it would violate #b - this is a very powerful hook and I don't want to make it easy or tempting to get at it. 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. You're adding a new public |
||
{ | ||
private FunctionDescriptor _descriptor; | ||
private ITriggeredFunctionInstanceFactory<TTriggerValue> _instanceFactory; | ||
|
@@ -30,16 +31,46 @@ public FunctionDescriptor Function | |
} | ||
} | ||
|
||
public async Task<FunctionResult> TryExecuteAsync(TriggeredFunctionData input, CancellationToken cancellationToken) | ||
public Task<FunctionResult> TryExecuteAsync(TriggeredFunctionData input, CancellationToken cancellationToken) | ||
{ | ||
return TryExecuteAsync(input, cancellationToken, null); | ||
} | ||
|
||
public async Task<FunctionResult> TryExecuteAsync(TriggeredFunctionData input, CancellationToken cancellationToken, Func<Func<Task>, Task> hook) | ||
{ | ||
IFunctionInstance instance = _instanceFactory.Create((TTriggerValue)input.TriggerValue, input.ParentId); | ||
if (hook != null) | ||
{ | ||
IFunctionInvoker invoker = new InvokeWrapper(instance.Invoker, hook); | ||
instance = new FunctionInstance(instance.Id, instance.ParentId, instance.Reason, instance.BindingSource, invoker, instance.FunctionDescriptor); | ||
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 is bypassing the instance factory. I don't think we should do that. 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. Why not? This doesn't need any factory services. What did you have in mind? An alternative approaches could be to make Invoker a mutable property and just change it on the existing instance. 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. If we have a factory pattern, then all creation should go through it. You might sidestep this problem by having the factory interface take the TriggeredFunctionData so it has access to the value as well as the callback. The interface is internal. |
||
} | ||
|
||
IDelayedException exception = await _executor.TryExecuteAsync(instance, cancellationToken); | ||
|
||
FunctionResult result = exception != null ? | ||
new FunctionResult(exception.Exception) | ||
new FunctionResult(exception.Exception) | ||
: new FunctionResult(true); | ||
|
||
return result; | ||
} | ||
|
||
private class InvokeWrapper : IFunctionInvoker | ||
{ | ||
private readonly IFunctionInvoker _inner; | ||
private readonly Func<Func<Task>, Task> _hook; | ||
|
||
public InvokeWrapper(IFunctionInvoker inner, Func<Func<Task>, Task> hook) | ||
{ | ||
_inner = inner; | ||
_hook = hook; | ||
} | ||
public IReadOnlyList<string> ParameterNames => _inner.ParameterNames; | ||
|
||
public Task InvokeAsync(object[] arguments) | ||
{ | ||
Func<Task> inner = () => _inner.InvokeAsync(arguments); | ||
return _hook(inner); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using Microsoft.Azure.WebJobs.Host.Executors; | ||
using Xunit; | ||
using Microsoft.Azure.WebJobs.Host.Protocols; | ||
using Microsoft.Azure.WebJobs.Host.Triggers; | ||
using System.Threading.Tasks; | ||
using System.Threading; | ||
using System.Text; | ||
using Moq; | ||
|
||
namespace Microsoft.Azure.WebJobs.Host.UnitTests.Executors | ||
{ | ||
public class TriggeredFunctionExecutorTests | ||
{ | ||
// Test ITriggeredFunctionExecutorWithHook | ||
[Fact] | ||
public async Task TestHook() | ||
{ | ||
StringBuilder sb = new StringBuilder(); | ||
|
||
var descr = new FunctionDescriptor(); | ||
|
||
// IFunctionExecutor just passes through to Invoker. | ||
var mockExecutor = new Mock<IFunctionExecutor>(); | ||
mockExecutor.Setup(m => m.TryExecuteAsync(It.IsAny<IFunctionInstance>(), It.IsAny<CancellationToken>())). | ||
Returns<IFunctionInstance, CancellationToken>((x, y) => | ||
{ | ||
sb.Append("2>"); | ||
x.Invoker.InvokeAsync(null).Wait(); | ||
sb.Append("<6"); | ||
return Task.FromResult<IDelayedException>(null); | ||
}); | ||
IFunctionExecutor executor = mockExecutor.Object; | ||
|
||
var mockInvoker = new Mock<IFunctionInvoker>(); | ||
mockInvoker.Setup(m => m.InvokeAsync(null)).Returns(() => | ||
{ | ||
sb.Append("4"); | ||
return Task.CompletedTask; | ||
} | ||
); | ||
IFunctionInvoker innerInvoker = mockInvoker.Object; | ||
|
||
IFunctionInstance inner = new FunctionInstance(Guid.NewGuid(), null, ExecutionReason.HostCall, null, innerInvoker, null); | ||
|
||
var mockInstanceFactory = new Mock<ITriggeredFunctionInstanceFactory<int>>(); | ||
mockInstanceFactory.Setup(m => m.Create(It.IsAny<int>(), null)).Returns(inner); | ||
ITriggeredFunctionInstanceFactory<int> instanceFactory = mockInstanceFactory.Object; | ||
|
||
var trigger = new TriggeredFunctionExecutor<int>(descr, executor, instanceFactory); | ||
|
||
var trigger2 = (ITriggeredFunctionExecutorWithHook)trigger; | ||
|
||
|
||
|
||
Func<Func<Task>, Task> hook = async (x) => { | ||
sb.Append("3>"); | ||
await x(); | ||
sb.Append("<5"); | ||
}; | ||
|
||
sb.Append("1>"); | ||
await trigger2.TryExecuteAsync(new TriggeredFunctionData { TriggerValue = 123 }, CancellationToken.None, hook); | ||
sb.Append("<7"); | ||
|
||
Assert.Equal("1>2>3>4<5<6<7", sb.ToString()); | ||
} | ||
} | ||
} |
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.
How does a custom trigger specify this hook?
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.
The binding provides a listener which is in full control of making calls on ITriggerFunctionExecutor*. So it's imperative calls; not registered.