Function Filter changes #1292

mathewc · 2017-08-11T16:02:37Z

Taking care of some loose ends with the new Function Invocation Filters feature. Main changes in this review:

adding a new filter type for errors/exceptions (see FunctionExceptionFilterAttribute and related classes)
temporarily removing some of the stuff that was added for Azure Functions integration. We'll re-add that back when we address Invocation Filter Integration azure-functions-host#1761
bug fixes
some attribute/interface renames

mathewc · 2017-08-11T16:06:48Z

src/Microsoft.Azure.WebJobs.Host/Executors/FunctionInstance.cs

@@ -51,6 +52,18 @@ public IFunctionInvoker Invoker
            get { return _invoker; }
        }

+        public object JobInstance


The job instance was needed earlier in the pipeline for exception filters, so I'm moving to a create/cache model so other places in the pipeline can access the cached instance

mathewc · 2017-08-11T16:13:24Z

src/Microsoft.Azure.WebJobs.Host/Filters/FunctionExceptionContext.cs

+    /// <summary>
+    /// Base context class for <see cref="IFunctionExceptionFilter.OnExceptionAsync(FunctionExceptionContext, System.Threading.CancellationToken)"/>.
+    /// </summary>
+    public class FunctionExceptionContext : FunctionFilterContext


Because exception filters can run before any input arguments are bound, the base class isn't FunctionInvocationContext (i.e. no Arguments dictionary). We might try to conditionally expose the arguments if the exception happened after they were bound. Thoughts?

I think this is good. We can always add more later. It's safer to not have arguments yet - people may come to rely on them and break when they get an early exception.

In reply to: 132725208 [](ancestors = 132725208)

mathewc · 2017-08-11T16:15:07Z

src/Microsoft.Azure.WebJobs.Host/Filters/FunctionExecutedContext.cs

+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the MIT License. See License.txt in the project root for license information.
+
+using System;


I moved all the Filters stuff under a new Host/Filters directory. That's why many of these show as additions instead of modifications.

mathewc · 2017-08-11T16:21:43Z

src/Microsoft.Azure.WebJobs.Host/Filters/FunctionInvocationContext.cs

+        /// <summary>
+        /// Gets the property bag that can be used to pass information between filters.
+        /// </summary>
+        public IDictionary<string, object> Properties { get; }


I'm thinking I need to move this to the base FunctionFilterContext so exception filters also have access to this property bag. I think all filters should have access to the shared bag for the invocation scope. In ASP.NET, this can be done via ActionContext.HttpContext.Items which is available to all filter types

Agreed - I think this should be on the base class.

In reply to: 132727087 [](ancestors = 132727087)

Moved to base class and added test

mathewc · 2017-08-11T16:22:37Z

src/Microsoft.Azure.WebJobs.Host/Filters/FunctionInvocationFilterAttribute.cs

 {
    /// <summary>
    /// Base class for declarative function invocation filters.
    /// </summary>
-    public abstract class InvocationFilterAttribute : Attribute, IFunctionInvocationFilter
+    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = true, Inherited = true)]
+    public abstract class FunctionInvocationFilterAttribute : Attribute, IFunctionInvocationFilter


Renaming so attribute and interface names are better in sync. Other filters follow the same pattern (IFunctionXXXFilter, FunctionXXXFilterAttribute)

mathewc · 2017-08-11T16:24:08Z

src/Microsoft.Azure.WebJobs.Host/FunctionExecutedContext.cs

-// Copyright (c) .NET Foundation. All rights reserved.
-// Licensed under the MIT License. See License.txt in the project root for license information.
-
-using Microsoft.Azure.WebJobs.Host.Executors;


These files all moved to the Filters directory

mathewc · 2017-08-11T16:26:17Z

test/Microsoft.Azure.WebJobs.Host.EndToEndTests/InvocationFilterEndToEndTests.cs

-// Licensed under the MIT License. See License.txt in the project root for license information.
-
-using System;
-using System.Collections.Generic;


All of these test scenarios are now covered via the unit test suite more concisely

mathewc · 2017-08-11T16:27:31Z

test/Microsoft.Azure.WebJobs.Host.UnitTests/Common/BindToGenericItemTests.cs

@@ -35,15 +35,8 @@ public void TestConcreteTypeNoConverter()
            TestWorker<ConfigConcreteTypeNoConverter>();
        }

-        public class ConfigConcreteTypeNoConverter : IExtensionConfigProvider, ITest<ConfigConcreteTypeNoConverter>
+        public class ConfigConcreteTypeNoConverter : BindingPathAttribute.Extension, ITest<ConfigConcreteTypeNoConverter>


Merged in tests that Mike added in Hamza's branch

MikeStall · 2017-08-11T16:38:03Z

src/Microsoft.Azure.WebJobs.Host/Executors/FunctionInstance.cs

+            {
+                if (_jobInstance == null)
+                {
+                    _jobInstance = Invoker.CreateInstance();


_jobInstance = Invoker.CreateInstance(); [](start = 20, length = 40)

Can we just create this upfront in the ctor?

Ensures no races.

lets the field be read-only like the others.

It gives us a more predictable location for exceptions coming from IJobActivator.

Actually, maybe just remove this whole property and put JobInstance on the ParameterHelper (where the other instantiated arguments live). That will solve of these issues.

In reply to: 132730667 [](ancestors = 132730667)

Good idea, done.

MikeStall · 2017-08-11T16:41:14Z

src/Microsoft.Azure.WebJobs.Host/Filters/FunctionInvocationContext.cs

+        /// <summary>
+        /// Gets the function arguments.
+        /// </summary>
+        public IReadOnlyDictionary<string, object> Arguments { get; }


string [](start = 35, length = 6)

Nit - can we comment that the key is the parameter name?

fabiocav · 2017-08-11T16:41:21Z

src/Microsoft.Azure.WebJobs.Host/Filters/FunctionExceptionContext.cs

+        /// <summary>
+        /// Gets the <see cref="Exception"/> that caused the function invocation to fail.
+        /// </summary>
+        public Exception Exception { get; }


We need to capture the ExceptionDispatchInfo in this context.

Will make both properties available as in ASP.NET

MikeStall · 2017-08-11T16:47:56Z

src/Microsoft.Azure.WebJobs.Host/Filters/IFunctionInvocationFilter.cs

@@ -4,23 +4,24 @@
 using System.Threading;
 using System.Threading.Tasks;

-namespace Microsoft.Azure.WebJobs.Host
+namespace Microsoft.Azure.WebJobs.Host.Filters


Filters [](start = 39, length = 7)

I'd vote against the sub namespace here. Function authors will be directly consuming this interface and itd be good to be top-level to be more discoverable.

Moved all back into the Host namespace

MikeStall · 2017-08-11T16:58:56Z

src/Microsoft.Azure.WebJobs.Host/Executors/FunctionExecutor.cs

+        /// for filters with pre/post methods (e.g. <see cref="IFunctionInvocationFilter"/>) the executing portion
+        /// of the filters runs in the reverse order.
+        /// </summary>
+        private static List<TFilter> GetFilters<TFilter>(IExtensionRegistry extensionRegistry, FunctionDescriptor functionDescriptor, object instance) where TFilter : class


class [](start = 167, length = 5)

Could this be more specific. IFunctionFilter?

MikeStall · 2017-08-11T17:05:41Z

src/Microsoft.Azure.WebJobs.Host/Executors/FunctionExecutor.cs

@@ -153,6 +159,19 @@ public HostOutputMessage HostOutputMessage
            return exceptionInfo != null ? new ExceptionDispatchInfoDelayedException(exceptionInfo) : null;
        }

+        private async Task InvokeExceptionFiltersAsync(Exception exception, IFunctionInstance functionInstance, CancellationToken cancellationToken)
+        {
+            var exceptionFilters = GetFilters<IFunctionExceptionFilter>(_extensionRegistry, functionInstance.FunctionDescriptor, functionInstance.JobInstance);


functionInstance.JobInstance [](start = 129, length = 28)

I think there's a bug here: currently, JobInstance is lazy; so this could call IJobActivator which could throw; and then we'd miss the exception.

JobActivator is no longer lazy - I'm creating it up front early. It is now encapsulated in the invocation try/catch so errors are handled, and I've added a test.

MikeStall · 2017-08-11T17:07:35Z

src/Microsoft.Azure.WebJobs.Host/Executors/FunctionExecutor.cs

@@ -31,12 +31,12 @@ internal class FunctionExecutor : IFunctionExecutor
        private readonly IAsyncCollector<FunctionInstanceLogEntry> _functionEventCollector;
        private readonly ILogger _logger;
        private readonly ILogger _resultsLogger;
-        private readonly IJobInvoker _jobInvoker;
+        private readonly IExtensionRegistry _extensionRegistry;


IExtensionRegistry [](start = 25, length = 18)

We shouldn't be passing this object around - too high level (same objection I had to passing JobHostConfig or even JobHost).

No longer referencing extension registry in executor now. Passing in collection of global filters.

MikeStall · 2017-08-11T17:09:54Z

src/Microsoft.Azure.WebJobs.Host/Executors/FunctionExecutor.cs

+                var exceptionContext = new FunctionExceptionContext(functionInstance.Id, functionInstance.FunctionDescriptor.ShortName, _logger, exception);
+                foreach (var exceptionFilter in exceptionFilters)
+                {
+                    await exceptionFilter.OnExceptionAsync(exceptionContext, cancellationToken);


OnExceptionAsync [](start = 42, length = 16)

This is user code - what should we do if it throws?

Keep executing other eh filters?

Does that become the new FunctionResult?
Throwing from an EH filter probably ought to be similar to throwing from a Post filter.

Now handling this the same way as we do for invocation filters. We keep track of the last exception, keep executing filters, and the last exception wins

MikeStall · 2017-08-11T17:19:20Z

src/Microsoft.Azure.WebJobs.Host/Executors/FunctionExecutor.cs

@@ -130,8 +136,8 @@ public HostOutputMessage HostOutputMessage
                    logCompletedCancellationToken = cancellationToken;
                }

-                await NotifyCompleteAsync(fastItem, functionCompletedMessage.Arguments, exceptionInfo);
-                _resultsLogger?.LogFunctionResult(fastItem);
+                await NotifyCompleteAsync(instanceLogEntry, functionCompletedMessage.Arguments, exceptionInfo);


NotifyCompleteAsync [](start = 22, length = 19)

This is the "Stop Billing" event. An exception coming from the new call to InvokeExceptionFiltersAsync (which is feasible since it calls user code) might skip this. The call to InvokeExceptionFiltersAsync should probably be wrapped in a try/catch.

InvokeExceptionFiltersAsync is now handling exceptions. That said, this end event should probably have been in a finally block to begin with...

fabiocav · 2017-08-11T17:20:11Z

Why are we targeting v2.x for this work? I thought the original work was based on dev and we'd backport.

MikeStall · 2017-08-11T17:21:44Z

src/Microsoft.Azure.WebJobs.Host/Executors/FunctionExecutor.cs

                // Create a source specifically for timeouts
                using (CancellationTokenSource timeoutTokenSource = new CancellationTokenSource())
                {
                    TimeoutAttribute timeoutAttribute = instance.FunctionDescriptor.TimeoutAttribute;
                    bool throwOnTimeout = timeoutAttribute == null ? false : timeoutAttribute.ThrowOnTimeout;
-                    var timer = StartFunctionTimeout(instance, timeoutAttribute, timeoutTokenSource, traceWriter, logger);
+                    var timer = StartFunctionTimeout(instance, timeoutAttribute, timeoutTokenSource, traceWriter, _logger);


StartFunctionTimeout [](start = 32, length = 20)

The new EH Filter won't be covered by this timeout. (Although neither is parameter binding - so maybe this timeout is just too low in the stack).

I'll track the timeout issue in a bug. I have a list of some of the other remaining items we want to address, and I'll include this

mathewc · 2017-08-11T18:44:06Z

@fabiocav I originally targeted this branch because I was doing the Azure Functions integration at the same time, and AF isn't on Core yet. Anyhow, I'll get all this into Core as well.

mathewc · 2017-08-12T00:17:44Z

Merged

dnfclas added the cla-already-signed label Aug 11, 2017

mathewc commented Aug 11, 2017

View reviewed changes

mathewc force-pushed the filter_updates branch from 5f95baf to 7617c92 Compare August 11, 2017 16:31

mathewc requested a review from MikeStall August 11, 2017 16:32

MikeStall reviewed Aug 11, 2017

View reviewed changes

fabiocav reviewed Aug 11, 2017

View reviewed changes

MikeStall reviewed Aug 11, 2017

View reviewed changes

mathewc force-pushed the filter_updates branch 2 times, most recently from b4ecd5e to b42dfb3 Compare August 11, 2017 22:19

Function Filter changes

ff69f34

mathewc force-pushed the filter_updates branch from b42dfb3 to ff69f34 Compare August 12, 2017 00:08

mathewc closed this Aug 12, 2017

mathewc deleted the filter_updates branch May 25, 2018 17:28

Function Filter changes #1292

Function Filter changes #1292

Conversation

mathewc commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeStall Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeStall Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

mathewc Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiocav commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc commented Aug 11, 2017

mathewc commented Aug 12, 2017

mathewc Aug 11, 2017 •

edited

Loading

MikeStall Aug 11, 2017 •

edited

Loading

MikeStall Aug 11, 2017 •

edited

Loading

mathewc Aug 11, 2017 •

edited

Loading