Provide a hook for invoke #1167
Conversation
Resolves #1140
| 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.
This is bypassing the instance factory. I don't think we should do that.
There was a problem hiding this comment.
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.
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.
| 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.
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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
| /// </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> |
There was a problem hiding this comment.
How does a custom trigger specify this hook?
There was a problem hiding this comment.
The binding provides a listener which is in full control of making calls on ITriggerFunctionExecutor*. So it's imperative calls; not registered.
|
Closing, we'll go with #1178 instead |
Resolves #1140