Disposal improvements for ScriptHost

mathewc · mathewc · commit e3e2b2aec4b6 · 2017-10-23T16:50:30.000-07:00
diff --git a/src/WebJobs.Script.WebHost/Diagnostics/FunctionInstanceLogger.cs b/src/WebJobs.Script.WebHost/Diagnostics/FunctionInstanceLogger.cs
@@ -79,8 +79,7 @@ internal FunctionInstanceLogger(
                         // This exception will cause the function to not get executed.
                         throw new InvalidOperationException($"Missing function.json for '{shortName}'.");
                     }
-                    FunctionLogger logInfo = descr.Invoker.LogInfo;
-                    state = new FunctionInstanceMonitor(descr.Metadata, _metrics, item.FunctionInstanceId, logInfo);
+                    state = new FunctionInstanceMonitor(descr.Metadata, _metrics, item.FunctionInstanceId, descr.Invoker.FunctionLogger);
 
                     item.Properties[Key] = state;
 
diff --git a/src/WebJobs.Script/Binding/FunctionTraceBinderProvider.cs b/src/WebJobs.Script/Binding/FunctionTraceBinderProvider.cs
@@ -24,9 +24,6 @@ public static void Create(ExtensionConfigContext context)
             IBindingProvider inner = (IBindingProvider)Activator.CreateInstance(traceBinderType, loggerFactory);
 
             Func<string, FunctionDescriptor> funcLookup = context.Config.GetService<Func<string, FunctionDescriptor>>();
-
-            // Func<string, FunctionDescriptor> funcLookup = null;
-
             if (funcLookup != null && inner != null)
             {
                 IBindingProvider wrapper = new Wrapper(inner, funcLookup);
@@ -86,10 +83,10 @@ private async Task<IValueProvider> WrapAsync(IValueProvider result, ValueBinding
                     {
                         var shortName = context.FunctionContext.MethodName;
                         FunctionDescriptor descr = _parent._funcLookup(shortName);
-                        var logInfo = descr.Invoker.LogInfo;
+                        var functionLogger = descr.Invoker.FunctionLogger;
 
                         // This is the critical call
-                        trace = logInfo.CreateUserTraceWriter(trace);
+                        trace = functionLogger.CreateUserTraceWriter(trace);
 
                         return new SimpleValueProvider(typeof(TraceWriter), trace, result.ToInvokeString());
                     }
diff --git a/src/WebJobs.Script/Description/FunctionInvokerBase.cs b/src/WebJobs.Script/Description/FunctionInvokerBase.cs
@@ -23,16 +23,11 @@ public abstract class FunctionInvokerBase : IFunctionInvoker, IDisposable
         private bool _disposed = false;
         private IDisposable _fileChangeSubscription;
 
-        internal FunctionInvokerBase(ScriptHost host, FunctionMetadata functionMetadata)
-            : this(host, functionMetadata, new FunctionLogger(host, functionMetadata.Name))
-        {
-        }
-
-        internal FunctionInvokerBase(ScriptHost host, FunctionMetadata functionMetadata, FunctionLogger logInfo)
+        internal FunctionInvokerBase(ScriptHost host, FunctionMetadata functionMetadata, string logDirName = null)
         {
             Host = host;
             Metadata = functionMetadata;
-            LogInfo = logInfo;
+            FunctionLogger = new FunctionLogger(host, logDirName ?? functionMetadata.Name);
         }
 
         protected static IDictionary<string, object> PrimaryHostTraceProperties { get; }
@@ -46,15 +41,15 @@ internal FunctionInvokerBase(ScriptHost host, FunctionMetadata functionMetadata,
 
         public ScriptHost Host { get; }
 
-        public FunctionLogger LogInfo { get; }
+        public FunctionLogger FunctionLogger { get; }
 
         public FunctionMetadata Metadata { get; }
 
-        protected TraceWriter TraceWriter => LogInfo.TraceWriter;
+        protected TraceWriter TraceWriter => FunctionLogger.TraceWriter;
 
-        protected ILogger Logger => LogInfo.Logger;
+        protected ILogger Logger => FunctionLogger.Logger;
 
-        public TraceWriter FileTraceWriter => LogInfo.FileTraceWriter;
+        public TraceWriter FileTraceWriter => FunctionLogger.FileTraceWriter;
 
         /// <summary>
         /// All unhandled invocation exceptions will flow through this method.
@@ -70,7 +65,7 @@ public virtual void OnError(Exception ex)
 
         protected virtual void TraceError(string errorMessage)
         {
-            LogInfo.TraceError(errorMessage);
+            FunctionLogger.TraceError(errorMessage);
         }
 
         protected bool InitializeFileWatcherIfEnabled()
@@ -155,7 +150,7 @@ internal void TraceCompilationDiagnostics(ImmutableArray<Diagnostic> diagnostics
                 return;
             }
 
-            TraceWriter traceWriter = LogInfo.TraceWriter;
+            TraceWriter traceWriter = FunctionLogger.TraceWriter;
             IDictionary<string, object> properties = PrimaryHostTraceProperties;
 
             if (!logTarget.HasFlag(LogTargets.User))
@@ -208,7 +203,7 @@ protected virtual void Dispose(bool disposing)
                 {
                     _fileChangeSubscription?.Dispose();
 
-                    (LogInfo.TraceWriter as IDisposable)?.Dispose();
+                    FunctionLogger.Dispose();
                 }
 
                 _disposed = true;
diff --git a/src/WebJobs.Script/Description/FunctionLogger.cs b/src/WebJobs.Script/Description/FunctionLogger.cs
@@ -1,6 +1,7 @@
 ﻿// 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.Collections.Generic;
 using System.Diagnostics;
 using Microsoft.Azure.WebJobs.Host;
@@ -11,8 +12,10 @@ namespace Microsoft.Azure.WebJobs.Script.Description
 {
     // Each function can get its own log stream.
     // Static per-function logging information.
-    public class FunctionLogger
+    public class FunctionLogger : IDisposable
     {
+        private bool _disposed = false;
+
         public FunctionLogger(ScriptHost host, string functionName, string logDirName = null)
         {
             // Function file logging is only done conditionally
@@ -82,5 +85,25 @@ public void LogFunctionResult(bool success, string invocationId, long elapsedMs)
             TraceWriter.Trace(message, traceWriterLevel, null);
             Logger?.Log(logLevel, new EventId(0), message, null, (s, e) => s);
         }
+
+        protected virtual void Dispose(bool disposing)
+        {
+            if (!_disposed)
+            {
+                if (disposing)
+                {
+                    (FileTraceWriter as IDisposable)?.Dispose();
+                    (TraceWriter as IDisposable)?.Dispose();
+                }
+
+                _disposed = true;
+            }
+        }
+
+        public void Dispose()
+        {
+            Dispose(true);
+            GC.SuppressFinalize(this);
+        }
     }
 }
diff --git a/src/WebJobs.Script/Description/IFunctionInvoker.cs b/src/WebJobs.Script/Description/IFunctionInvoker.cs
@@ -11,7 +11,7 @@ public interface IFunctionInvoker
         /// <summary>
         /// Gets logging information for this function.
         /// </summary>
-        FunctionLogger LogInfo { get; }
+        FunctionLogger FunctionLogger { get; }
 
         /// <summary>
         /// Invoke the function using the specified parameters.
diff --git a/src/WebJobs.Script/Description/Node/NodeFunctionInvoker.cs b/src/WebJobs.Script/Description/Node/NodeFunctionInvoker.cs
@@ -336,6 +336,16 @@ private async Task ReloadScriptAsync()
             }
         }
 
+        protected override void Dispose(bool disposing)
+        {
+            base.Dispose(disposing);
+
+            if (disposing)
+            {
+                _functionLoader.Dispose();
+            }
+        }
+
         private async Task<Dictionary<string, object>> CreateScriptExecutionContextAsync(object input, DataType dataType, TraceWriter traceWriter, FunctionInvocationContext invocationContext)
         {
             // create a TraceWriter wrapper that can be exposed to Node.js
diff --git a/src/WebJobs.Script/Description/Proxies/ProxyFunctionInvoker.cs b/src/WebJobs.Script/Description/Proxies/ProxyFunctionInvoker.cs
@@ -2,13 +2,9 @@
 // Licensed under the MIT License. See License.txt in the project root for license information.
 
 using System;
-using System.Collections.Generic;
 using System.Linq;
-using System.Management.Automation;
 using System.Net.Http;
-using System.Text;
 using System.Threading.Tasks;
-using Microsoft.Azure.AppService.Proxy.Client.Contract;
 
 namespace Microsoft.Azure.WebJobs.Script.Description
 {
@@ -17,7 +13,7 @@ internal class ProxyFunctionInvoker : FunctionInvokerBase
         private ProxyClientExecutor _proxyClient;
 
         public ProxyFunctionInvoker(ScriptHost host, FunctionMetadata functionMetadata, ProxyClientExecutor proxyClient)
-            : base(host, functionMetadata, new FunctionLogger(host, functionMetadata.Name, logDirName: "Proxy"))
+            : base(host, functionMetadata, logDirName: "Proxy")
         {
             _proxyClient = proxyClient;
         }
diff --git a/src/WebJobs.Script/GlobalSuppressions.cs b/src/WebJobs.Script/GlobalSuppressions.cs
@@ -229,4 +229,5 @@
 [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", MessageId = "Dir", Scope = "member", Target = "Microsoft.Azure.WebJobs.Script.Description.FunctionLogger.#.ctor(Microsoft.Azure.WebJobs.Script.ScriptHost,System.String,System.String)")]
 [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", MessageId = "Dir", Scope = "member", Target = "Microsoft.Azure.WebJobs.Script.IFunctionTraceWriterFactory.#Create(System.String,System.String)")]
 [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Scope = "member", Target = "Microsoft.Azure.WebJobs.Script.ScriptHost.#Create(Microsoft.Azure.WebJobs.Script.IScriptHostEnvironment,Microsoft.Azure.WebJobs.Script.Eventing.IScriptEventManager,Microsoft.Azure.WebJobs.Script.ScriptHostConfiguration,Microsoft.Azure.WebJobs.Script.Config.ScriptSettingsManager,Microsoft.Azure.WebJobs.Script.Description.ProxyClientExecutor)")]
-[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Reliability", "CA2001:AvoidCallingProblematicMethods", MessageId = "System.Reflection.Assembly.LoadFrom", Scope = "member", Target = "Microsoft.Azure.WebJobs.Script.Binding.ExtensionLoader.#LoadCustomExtensions(System.String)")]
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Reliability", "CA2001:AvoidCallingProblematicMethods", MessageId = "System.Reflection.Assembly.LoadFrom", Scope = "member", Target = "Microsoft.Azure.WebJobs.Script.Binding.ExtensionLoader.#LoadCustomExtensions(System.String)")]
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2213:DisposableFieldsShouldBeDisposed", MessageId = "<FunctionLogger>k__BackingField", Scope = "member", Target = "Microsoft.Azure.WebJobs.Script.Description.FunctionInvokerBase.#Dispose(System.Boolean)")]
diff --git a/src/WebJobs.Script/Host/PrimaryHostCoordinator.cs b/src/WebJobs.Script/Host/PrimaryHostCoordinator.cs
@@ -203,7 +203,10 @@ private void ResetLease()
 
         private void SetTimerInterval(TimeSpan interval, TimeSpan? dueTimeout = null)
         {
-            _timer.Change(dueTimeout ?? interval, interval);
+            if (!_disposed)
+            {
+                _timer.Change(dueTimeout ?? interval, interval);
+            }
         }
 
         private void TryReleaseLeaseIfOwned()
diff --git a/src/WebJobs.Script/Host/ScriptHost.cs b/src/WebJobs.Script/Host/ScriptHost.cs
diff --git a/src/WebJobs.Script/Host/ScriptHostManager.cs b/src/WebJobs.Script/Host/ScriptHostManager.cs
diff --git a/test/WebJobs.Script.Tests.Integration/Host/WebScriptHostManagerTests.cs b/test/WebJobs.Script.Tests.Integration/Host/WebScriptHostManagerTests.cs
diff --git a/test/WebJobs.Script.Tests.Shared/TestInvoker.cs b/test/WebJobs.Script.Tests.Shared/TestInvoker.cs

Original file line number	Diff line number	Diff line change
`@@ -79,8 +79,7 @@ internal FunctionInstanceLogger(`
`79`	`79`	`// This exception will cause the function to not get executed.`
`80`	`80`	`throw new InvalidOperationException($"Missing function.json for '{shortName}'.");`
`81`	`81`	`}`
`82`		`- FunctionLogger logInfo = descr.Invoker.LogInfo;`
`83`		`- state = new FunctionInstanceMonitor(descr.Metadata, _metrics, item.FunctionInstanceId, logInfo);`
	`82`	`+ state = new FunctionInstanceMonitor(descr.Metadata, _metrics, item.FunctionInstanceId, descr.Invoker.FunctionLogger);`
`84`	`83`
`85`	`84`	`item.Properties[Key] = state;`
`86`	`85`
Original file line number	Diff line number	Diff line change
`@@ -24,9 +24,6 @@ public static void Create(ExtensionConfigContext context)`
`24`	`24`	`IBindingProvider inner = (IBindingProvider)Activator.CreateInstance(traceBinderType, loggerFactory);`
`25`	`25`
`26`	`26`	`Func<string, FunctionDescriptor> funcLookup = context.Config.GetService<Func<string, FunctionDescriptor>>();`
`27`		`-`
`28`		`- // Func<string, FunctionDescriptor> funcLookup = null;`
`29`		`-`
`30`	`27`	`if (funcLookup != null && inner != null)`
`31`	`28`	`{`
`32`	`29`	`IBindingProvider wrapper = new Wrapper(inner, funcLookup);`
`@@ -86,10 +83,10 @@ private async Task<IValueProvider> WrapAsync(IValueProvider result, ValueBinding`
`86`	`83`	`{`
`87`	`84`	`var shortName = context.FunctionContext.MethodName;`
`88`	`85`	`FunctionDescriptor descr = _parent._funcLookup(shortName);`
`89`		`- var logInfo = descr.Invoker.LogInfo;`
	`86`	`+ var functionLogger = descr.Invoker.FunctionLogger;`
`90`	`87`
`91`	`88`	`// This is the critical call`
`92`		`- trace = logInfo.CreateUserTraceWriter(trace);`
	`89`	`+ trace = functionLogger.CreateUserTraceWriter(trace);`
`93`	`90`
`94`	`91`	`return new SimpleValueProvider(typeof(TraceWriter), trace, result.ToInvokeString());`
`95`	`92`	`}`
Original file line number	Diff line number	Diff line change
`@@ -336,6 +336,16 @@ private async Task ReloadScriptAsync()`
`336`	`336`	`}`
`337`	`337`	`}`
`338`	`338`
	`339`	`+ protected override void Dispose(bool disposing)`
	`340`	`+ {`
	`341`	`+ base.Dispose(disposing);`
	`342`	`+`
	`343`	`+ if (disposing)`
	`344`	`+ {`
	`345`	`+ _functionLoader.Dispose();`
	`346`	`+ }`
	`347`	`+ }`
	`348`	`+`
`339`	`349`	`private async Task<Dictionary<string, object>> CreateScriptExecutionContextAsync(object input, DataType dataType, TraceWriter traceWriter, FunctionInvocationContext invocationContext)`
`340`	`350`	`{`
`341`	`351`	`// create a TraceWriter wrapper that can be exposed to Node.js`
Original file line number	Diff line number	Diff line change
`@@ -2,13 +2,9 @@`
`2`	`2`	`// Licensed under the MIT License. See License.txt in the project root for license information.`
`3`	`3`
`4`	`4`	`using System;`
`5`		`-using System.Collections.Generic;`
`6`	`5`	`using System.Linq;`
`7`		`-using System.Management.Automation;`
`8`	`6`	`using System.Net.Http;`
`9`		`-using System.Text;`
`10`	`7`	`using System.Threading.Tasks;`
`11`		`-using Microsoft.Azure.AppService.Proxy.Client.Contract;`
`12`	`8`
`13`	`9`	`namespace Microsoft.Azure.WebJobs.Script.Description`
`14`	`10`	`{`
`@@ -17,7 +13,7 @@ internal class ProxyFunctionInvoker : FunctionInvokerBase`
`17`	`13`	`private ProxyClientExecutor _proxyClient;`
`18`	`14`
`19`	`15`	`public ProxyFunctionInvoker(ScriptHost host, FunctionMetadata functionMetadata, ProxyClientExecutor proxyClient)`
`20`		`- : base(host, functionMetadata, new FunctionLogger(host, functionMetadata.Name, logDirName: "Proxy"))`
	`16`	`+ : base(host, functionMetadata, logDirName: "Proxy")`
`21`	`17`	`{`
`22`	`18`	`_proxyClient = proxyClient;`
`23`	`19`	`}`
Original file line number	Diff line number	Diff line change
`@@ -203,7 +203,10 @@ private void ResetLease()`
`203`	`203`
`204`	`204`	`private void SetTimerInterval(TimeSpan interval, TimeSpan? dueTimeout = null)`
`205`	`205`	`{`
`206`		`- _timer.Change(dueTimeout ?? interval, interval);`
	`206`	`+ if (!_disposed)`
	`207`	`+ {`
	`208`	`+ _timer.Change(dueTimeout ?? interval, interval);`
	`209`	`+ }`
`207`	`210`	`}`
`208`	`211`
`209`	`212`	`private void TryReleaseLeaseIfOwned()`