Fix Http trigger disposal issue (#1537)

mathewc · mathewc · commit b9faed13e648 · 2017-06-07T11:35:09.000-07:00
diff --git a/src/WebJobs.Script.WebHost/Controllers/FunctionsController.cs b/src/WebJobs.Script.WebHost/Controllers/FunctionsController.cs
@@ -11,7 +11,6 @@
 using System.Web.Http.Controllers;
 using System.Web.Http.Dependencies;
 using Microsoft.Azure.WebJobs.Extensions.Http;
-using Microsoft.Azure.WebJobs.Script.Binding;
 using Microsoft.Azure.WebJobs.Script.Description;
 using Microsoft.Azure.WebJobs.Script.WebHost.Filters;
 using Microsoft.Azure.WebJobs.Script.WebHost.WebHooks;
diff --git a/src/WebJobs.Script.WebHost/Handlers/WebScriptHostHandler.cs b/src/WebJobs.Script.WebHost/Handlers/WebScriptHostHandler.cs
@@ -2,7 +2,6 @@
 // Licensed under the MIT License. See License.txt in the project root for license information.
 
 using System;
-using System.Linq;
 using System.Net;
 using System.Net.Http;
 using System.Threading;
@@ -13,19 +12,22 @@ namespace Microsoft.Azure.WebJobs.Script.WebHost.Handlers
 {
     public class WebScriptHostHandler : DelegatingHandler
     {
-        private readonly TimeSpan _hostTimeoutSeconds;
-        private readonly int _hostRunningPollIntervalMs;
+        public const int HostTimeoutSeconds = 30;
+        public const int HostPollingIntervalMilliseconds = 500;
+
+        private readonly int _hostTimeoutSeconds;
+        private readonly int _hostRunningPollIntervalMilliseconds;
         private readonly HttpConfiguration _config;
 
-        public WebScriptHostHandler(HttpConfiguration config, int hostTimeoutSeconds = 30, int hostRunningPollIntervalMS = 500)
+        public WebScriptHostHandler(HttpConfiguration config, int hostTimeoutSeconds = HostTimeoutSeconds, int hostPollingIntervalMilliseconds = HostPollingIntervalMilliseconds)
         {
             if (config == null)
             {
                 throw new ArgumentNullException("config");
             }
 
-            _hostRunningPollIntervalMs = hostRunningPollIntervalMS;
-            _hostTimeoutSeconds = new TimeSpan(0, 0, hostTimeoutSeconds);
+            _hostRunningPollIntervalMilliseconds = hostPollingIntervalMilliseconds;
+            _hostTimeoutSeconds = hostTimeoutSeconds;
             _config = config;
         }
 
@@ -55,23 +57,7 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
                 // If the host is not running, we'll wait a bit for it to fully
                 // initialize. This might happen if http requests come in while the
                 // host is starting up for the first time, or if it is restarting.
-                TimeSpan timeWaited = TimeSpan.Zero;
-                while (!scriptHostManager.CanInvoke() &&
-                        scriptHostManager.State != ScriptHostState.Error &&
-                        (timeWaited < _hostTimeoutSeconds))
-                {
-                    await Task.Delay(_hostRunningPollIntervalMs);
-                    timeWaited += TimeSpan.FromMilliseconds(_hostRunningPollIntervalMs);
-                }
-
-                // if the host is not ready after our wait time has expired return a 503
-                if (!scriptHostManager.CanInvoke())
-                {
-                    return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable)
-                    {
-                        Content = new StringContent("Function host is not running.")
-                    };
-                }
+                await scriptHostManager.DelayUntilHostReady(_hostTimeoutSeconds, _hostRunningPollIntervalMilliseconds);
             }
 
             return await base.SendAsync(request, cancellationToken);
diff --git a/src/WebJobs.Script.WebHost/WebScriptHostManager.cs b/src/WebJobs.Script.WebHost/WebScriptHostManager.cs
@@ -26,6 +26,7 @@
 using Microsoft.Azure.WebJobs.Script.Diagnostics;
 using Microsoft.Azure.WebJobs.Script.Eventing;
 using Microsoft.Azure.WebJobs.Script.WebHost.Diagnostics;
+using Microsoft.Azure.WebJobs.Script.WebHost.Handlers;
 using Microsoft.Extensions.Logging;
 
 namespace Microsoft.Azure.WebJobs.Script.WebHost
@@ -42,6 +43,8 @@ public class WebScriptHostManager : ScriptHostManager
         private readonly ScriptHostConfiguration _config;
         private readonly ISwaggerDocumentManager _swaggerDocumentManager;
         private readonly object _syncLock = new object();
+        private readonly int _hostTimeoutSeconds;
+        private readonly int _hostRunningPollIntervalMilliseconds;
 
         private readonly WebJobsSdkExtensionHookProvider _bindingWebHookProvider = new WebJobsSdkExtensionHookProvider();
 
@@ -57,13 +60,17 @@ public WebScriptHostManager(ScriptHostConfiguration config,
             ScriptSettingsManager settingsManager,
             WebHostSettings webHostSettings,
             IScriptHostFactory scriptHostFactory = null,
-            ISecretsRepositoryFactory secretsRepositoryFactory = null)
+            ISecretsRepositoryFactory secretsRepositoryFactory = null,
+            int hostTimeoutSeconds = WebScriptHostHandler.HostTimeoutSeconds,
+            int hostPollingIntervalMilliseconds = WebScriptHostHandler.HostPollingIntervalMilliseconds)
             : base(config, settingsManager, scriptHostFactory, eventManager)
         {
             _config = config;
             _metricsLogger = new WebHostMetricsLogger();
             _exceptionHandler = new WebScriptHostExceptionHandler(this);
             _webHostSettings = webHostSettings;
+            _hostTimeoutSeconds = hostTimeoutSeconds;
+            _hostRunningPollIntervalMilliseconds = hostPollingIntervalMilliseconds;
 
             var systemEventGenerator = config.HostConfig.GetService<IEventGenerator>() ?? new EventGenerator();
             var systemTraceWriter = new SystemTraceWriter(systemEventGenerator, settingsManager, TraceLevel.Verbose);
@@ -160,10 +167,12 @@ public IReadOnlyDictionary<IHttpRoute, FunctionDescriptor> HttpFunctions
 
         public async Task<HttpResponseMessage> HandleRequestAsync(FunctionDescriptor function, HttpRequestMessage request, CancellationToken cancellationToken)
         {
+            // ensure that the host is ready to process requests
+            await DelayUntilHostReady(_hostTimeoutSeconds, _hostRunningPollIntervalMilliseconds);
+
             // All authentication is assumed to have been done on the request
             // BEFORE this method is called
-
-            Dictionary<string, object> arguments = GetFunctionArguments(function, request);
+            var arguments = GetFunctionArguments(function, request);
 
             // Suspend the current synchronization context so we don't pass the ASP.NET
             // context down to the function.
@@ -173,11 +182,11 @@ public async Task<HttpResponseMessage> HandleRequestAsync(FunctionDescriptor fun
                 // record details about the request.
                 ILoggerFactory loggerFactory = _config.HostConfig.GetService<ILoggerFactory>();
                 ILogger logger = loggerFactory.CreateLogger(LogCategories.Function);
-                using (logger.BeginScope(
-                    new Dictionary<string, object>()
-                    {
-                        [ScriptConstants.LoggerHttpRequest] = request
-                    }))
+                var scopeState = new Dictionary<string, object>()
+                {
+                    [ScriptConstants.LoggerHttpRequest] = request
+                };
+                using (logger.BeginScope(scopeState))
                 {
                     await Instance.CallAsync(function.Name, arguments, cancellationToken);
                 }
@@ -515,5 +524,29 @@ public override void Shutdown()
             Stop();
             HostingEnvironment.InitiateShutdown();
         }
+
+        public async Task DelayUntilHostReady(int timeoutSeconds = WebScriptHostHandler.HostTimeoutSeconds, int pollingIntervalMilliseconds = WebScriptHostHandler.HostPollingIntervalMilliseconds, bool throwOnFailure = true)
+        {
+            TimeSpan timeout = TimeSpan.FromSeconds(timeoutSeconds);
+            TimeSpan delay = TimeSpan.FromMilliseconds(pollingIntervalMilliseconds);
+            TimeSpan timeWaited = TimeSpan.Zero;
+
+            while (!CanInvoke() &&
+                    State != ScriptHostState.Error &&
+                    (timeWaited < timeout))
+            {
+                await Task.Delay(delay);
+                timeWaited += delay;
+            }
+
+            if (throwOnFailure && !CanInvoke())
+            {
+                var errorResponse = new HttpResponseMessage(HttpStatusCode.ServiceUnavailable)
+                {
+                    Content = new StringContent("Function host is not running.")
+                };
+                throw new HttpResponseException(errorResponse);
+            }
+        }
     }
 }
diff --git a/src/WebJobs.Script/Host/ScriptHostManager.cs b/src/WebJobs.Script/Host/ScriptHostManager.cs
@@ -15,8 +15,6 @@
 using Microsoft.Azure.WebJobs.Script.Config;
 using Microsoft.Azure.WebJobs.Script.Diagnostics;
 using Microsoft.Azure.WebJobs.Script.Eventing;
-using Microsoft.Azure.WebJobs.Script.Eventing.File;
-using Microsoft.Azure.WebJobs.Script.IO;
 using Microsoft.Extensions.Logging;
 
 namespace Microsoft.Azure.WebJobs.Script
diff --git a/test/WebJobs.Script.Tests.Integration/Host/WebScriptHostManagerTests.cs b/test/WebJobs.Script.Tests.Integration/Host/WebScriptHostManagerTests.cs
@@ -6,8 +6,10 @@
 using System.Diagnostics;
 using System.IO;
 using System.Linq;
+using System.Net;
 using System.Net.Http;
 using System.Text.RegularExpressions;
+using System.Threading;
 using System.Threading.Tasks;
 using System.Web.Http;
 using System.Web.Http.Routing;
@@ -28,12 +30,40 @@ public class WebScriptHostManagerTests : IClassFixture<WebScriptHostManagerTests
     {
         private readonly ScriptSettingsManager _settingsManager;
         private readonly TempDirectory _secretsDirectory = new TempDirectory();
+        private readonly WebScriptHostManager _hostManager;
+        private readonly Mock<IScriptHostFactory> _mockScriptHostFactory;
+        private ScriptHostConfiguration _config;
         private WebScriptHostManagerTests.Fixture _fixture;
 
         public WebScriptHostManagerTests(WebScriptHostManagerTests.Fixture fixture)
         {
             _fixture = fixture;
             _settingsManager = ScriptSettingsManager.Instance;
+
+            string functionTestDir = Path.Combine(_fixture.TestFunctionRoot, Guid.NewGuid().ToString());
+            Directory.CreateDirectory(functionTestDir);
+            string logDir = Path.Combine(_fixture.TestLogsRoot, Guid.NewGuid().ToString());
+            string secretsDir = Path.Combine(_fixture.TestSecretsRoot, Guid.NewGuid().ToString());
+
+            _config = new ScriptHostConfiguration
+            {
+                RootLogPath = logDir,
+                RootScriptPath = functionTestDir,
+                FileLoggingMode = FileLoggingMode.Always,
+            };
+
+            var secretsRepository = new FileSystemSecretsRepository(_secretsDirectory.Path);
+            SecretManager secretManager = new SecretManager(_settingsManager, secretsRepository, NullTraceWriter.Instance, null);
+            WebHostSettings webHostSettings = new WebHostSettings
+            {
+                SecretsPath = _secretsDirectory.Path
+            };
+
+            var mockEventManager = new Mock<IScriptEventManager>();
+            _mockScriptHostFactory = new Mock<IScriptHostFactory>();
+
+            _hostManager = new WebScriptHostManager(_config, new TestSecretManagerFactory(secretManager), mockEventManager.Object,
+                _settingsManager, webHostSettings, _mockScriptHostFactory.Object, new DefaultSecretsRepositoryFactory(), 2, 500);
         }
 
         [Fact]
@@ -142,48 +172,41 @@ public async Task EmptyHost_StartsSuccessfully()
         [Fact]
         public async Task MultipleHostRestarts()
         {
-            string functionTestDir = Path.Combine(_fixture.TestFunctionRoot, Guid.NewGuid().ToString());
-            Directory.CreateDirectory(functionTestDir);
-            string logDir = Path.Combine(_fixture.TestLogsRoot, Guid.NewGuid().ToString());
-            string secretsDir = Path.Combine(_fixture.TestSecretsRoot, Guid.NewGuid().ToString());
-
-            ScriptHostConfiguration config = new ScriptHostConfiguration
-            {
-                RootLogPath = logDir,
-                RootScriptPath = functionTestDir,
-                FileLoggingMode = FileLoggingMode.Always,
-            };
-
-            ISecretsRepository repository = new FileSystemSecretsRepository(_secretsDirectory.Path);
-            SecretManager secretManager = new SecretManager(_settingsManager, repository, NullTraceWriter.Instance, null);
-            WebHostSettings webHostSettings = new WebHostSettings();
-            webHostSettings.SecretsPath = _secretsDirectory.Path;
-
-            var mockEventManager = new Mock<IScriptEventManager>();
-            var factoryMock = new Mock<IScriptHostFactory>();
             int count = 0;
-            factoryMock.Setup(p => p.Create(It.IsAny<IScriptHostEnvironment>(), It.IsAny<IScriptEventManager>(), _settingsManager, config)).Callback(() =>
+            _mockScriptHostFactory.Setup(p => p.Create(It.IsAny<IScriptHostEnvironment>(), It.IsAny<IScriptEventManager>(), _settingsManager, _config)).Callback(() =>
             {
                 count++;
             }).Throws(new Exception("Kaboom!"));
 
-            ScriptHostManager hostManager = new WebScriptHostManager(config, new TestSecretManagerFactory(secretManager), mockEventManager.Object,
-                _settingsManager, webHostSettings, factoryMock.Object);
-
-            Task runTask = Task.Run(() => hostManager.RunAndBlock());
+            Task runTask = Task.Run(() => _hostManager.RunAndBlock());
 
             await TestHelpers.Await(() =>
             {
                 return count > 3;
             });
 
-            hostManager.Stop();
-            Assert.Equal(ScriptHostState.Default, hostManager.State);
+            _hostManager.Stop();
+            Assert.Equal(ScriptHostState.Default, _hostManager.State);
 
             // regression test: previously on multiple restarts we were recomposing
             // the writer on each restart, resulting in a nested chain of writers
             // increasing on each restart
-            Assert.Equal(typeof(SystemTraceWriter), config.TraceWriter.GetType());
+            Assert.Equal(typeof(SystemTraceWriter), _config.TraceWriter.GetType());
+        }
+
+        [Fact]
+        public async Task HandleRequestAsync_HostNotReady_Throws()
+        {
+            Assert.False(_hostManager.CanInvoke());
+
+            var ex = await Assert.ThrowsAsync<HttpResponseException>(async () =>
+            {
+                await _hostManager.HandleRequestAsync(null, new HttpRequestMessage(), CancellationToken.None);
+            });
+
+            Assert.Equal(HttpStatusCode.ServiceUnavailable, ex.Response.StatusCode);
+            var message = await ex.Response.Content.ReadAsStringAsync();
+            Assert.Equal("Function host is not running.", message);
         }
 
         [Fact]
diff --git a/test/WebJobs.Script.Tests.Integration/SamplesEndToEndRestartTests.cs b/test/WebJobs.Script.Tests.Integration/SamplesEndToEndRestartTests.cs
diff --git a/test/WebJobs.Script.Tests.Integration/SamplesEndToEndTests.cs b/test/WebJobs.Script.Tests.Integration/SamplesEndToEndTests.cs
@@ -34,7 +34,7 @@ namespace Microsoft.Azure.WebJobs.Script.Tests
 {
     public class SamplesEndToEndTests : IClassFixture<SamplesEndToEndTests.TestFixture>
     {
-        private const string MasterKey = "t8laajal0a1ajkgzoqlfv5gxr4ebhqozebw4qzdy";
+        internal const string MasterKey = "t8laajal0a1ajkgzoqlfv5gxr4ebhqozebw4qzdy";
 
         private readonly ScriptSettingsManager _settingsManager;
         private TestFixture _fixture;
diff --git a/test/WebJobs.Script.Tests.Integration/WebJobs.Script.Tests.Integration.csproj b/test/WebJobs.Script.Tests.Integration/WebJobs.Script.Tests.Integration.csproj
@@ -431,7 +431,6 @@
       <DesignTime>True</DesignTime>
       <DependentUpon>Resources.resx</DependentUpon>
     </Compile>
-    <Compile Include="SamplesEndToEndRestartTests.cs" />
     <Compile Include="SamplesEndToEndTests.cs" />
     <Compile Include="Host\ScriptHostManagerTests.cs" />
     <Compile Include="Host\StandbyModeTests.cs" />
diff --git a/test/WebJobs.Script.Tests.Shared/TestHelpers.cs b/test/WebJobs.Script.Tests.Shared/TestHelpers.cs
diff --git a/test/WebJobs.Script.Tests/Handlers/WebScriptHostHandlerTests.cs b/test/WebJobs.Script.Tests/Handlers/WebScriptHostHandlerTests.cs

Original file line number	Diff line number	Diff line change
`@@ -34,7 +34,7 @@ namespace Microsoft.Azure.WebJobs.Script.Tests`
`34`	`34`	`{`
`35`	`35`	`public class SamplesEndToEndTests : IClassFixture<SamplesEndToEndTests.TestFixture>`
`36`	`36`	`{`
`37`		`- private const string MasterKey = "t8laajal0a1ajkgzoqlfv5gxr4ebhqozebw4qzdy";`
	`37`	`+ internal const string MasterKey = "t8laajal0a1ajkgzoqlfv5gxr4ebhqozebw4qzdy";`
`38`	`38`
`39`	`39`	`private readonly ScriptSettingsManager _settingsManager;`
`40`	`40`	`private TestFixture _fixture;`