Skip to content
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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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>
Copy link
Member

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?

Copy link
Contributor Author

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.

/// <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
Expand Up @@ -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
Copy link
Member

@mathewc mathewc Jun 1, 2017

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
a) fulfilled some strict scenario requirements (it needs to be dispatched exactly at this spot in the pipeline);
b) was relatively isolated and had a clear 'opt-in' model.
This is a rocket-science case and I specifically didn't want it to get broad traction except from things that really needed it. I didn't want to create a matrix of feature interactions.

Copy link
Member

@mathewc mathewc Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative to consider would be to extend the existing TriggeredFunctionData parameter by adding your callback on there. Easy for consumer to specify the callback, and below you just check for it and use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're adding a new public ITriggeredFunctionExecutorWithHook interface that we need to maintain going forward. It feels pretty hacky. If we really need to add this capability then I think we should add it in a clean way and commit to it.

{
private FunctionDescriptor _descriptor;
private ITriggeredFunctionInstanceFactory<TTriggerValue> _instanceFactory;
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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);
}
}
}
}
1 change: 1 addition & 0 deletions src/Microsoft.Azure.WebJobs.Host/WebJobs.Host.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,7 @@
<Compile Include="Diagnostics\ExceptionFormatter.cs" />
<Compile Include="Executors\CompositeFunctionEventCollector.cs" />
<Compile Include="Executors\FunctionInstanceTraceWriter.cs" />
<Compile Include="Executors\ITriggeredFunctionExecutorWithHook.cs" />
<Compile Include="Extensions\CloudQueueMessageExtensions.cs" />
<Compile Include="Extensions\IJobHostMetadataProvider.cs" />
<Compile Include="Extensions\JobHostMetadataProvider.cs" />
Expand Down
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());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ public void WebJobsHostPublicSurface_LimitedToSpecificTypes()
"AmbientConnectionStringProvider",
"IExtensionRegistryExtensions",
"ITriggeredFunctionExecutor",
"ITriggeredFunctionExecutorWithHook",
"ListenerFactoryContext",
"BindingTemplateSource",
"TriggeredFunctionData",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@
<Compile Include="Blobs\UncompletedAsyncResult.cs" />
<Compile Include="Blobs\CompletedAsyncResult.cs" />
<Compile Include="Blobs\WatchableReadStreamTests.cs" />
<Compile Include="Executors\TriggeredFunctionExecutorTests.cs" />
<Compile Include="ExtensionConfigContextTests.cs" />
<Compile Include="Loggers\ApplicationInsightsLoggerProviderTests.cs" />
<Compile Include="Loggers\DefaultTelemetryClientFactoryTests.cs" />
Expand Down