diff --git a/src/AspNetCoreModuleV2/AspNetCore/src/applicationinfo.cpp b/src/AspNetCoreModuleV2/AspNetCore/src/applicationinfo.cpp index b1bcc464c3..99251ddc4a 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/src/applicationinfo.cpp +++ b/src/AspNetCoreModuleV2/AspNetCore/src/applicationinfo.cpp @@ -187,12 +187,6 @@ APPLICATION_INFO::EnsureApplicationCreated( // one optimization for failure scenario is to reduce the lock scope SRWExclusiveLock lock(m_srwLock); - if (m_fDoneAppCreation) - { - // application is NULL and CreateApplication failed previously - FINISHED(E_APPLICATION_ACTIVATION_EXEC_FAILURE); - } - else { if (m_pApplication != NULL) { diff --git a/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpContext.FeatureCollection.cs b/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpContext.FeatureCollection.cs index 05c376de0e..4c2d4342b9 100644 --- a/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpContext.FeatureCollection.cs +++ b/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpContext.FeatureCollection.cs @@ -182,7 +182,7 @@ namespace Microsoft.AspNetCore.Server.IIS.Core bool IHttpResponseFeature.HasStarted => HasResponseStarted; - bool IHttpUpgradeFeature.IsUpgradableRequest => _server.IsWebSocketAvailable(_pInProcessHandler); + bool IHttpUpgradeFeature.IsUpgradableRequest => true; bool IFeatureCollection.IsReadOnly => false; @@ -205,7 +205,11 @@ namespace Microsoft.AspNetCore.Server.IIS.Core return null; } - return NativeMethods.HttpTryGetServerVariable(_pInProcessHandler, variableName, out var value) ? value : null; + // Synchronize access to native methods that might run in parallel with IO loops + lock (_contextLock) + { + return NativeMethods.HttpTryGetServerVariable(_pInProcessHandler, variableName, out var value) ? value : null; + } } } @@ -265,7 +269,7 @@ namespace Microsoft.AspNetCore.Server.IIS.Core _hasRequestReadingStarted = false; // Upgrade async will cause the stream processing to go into duplex mode - AsyncIO = new WebSocketsAsyncIOEngine(_pInProcessHandler); + AsyncIO = new WebSocketsAsyncIOEngine(_contextLock, _pInProcessHandler); await InitializeResponse(flushHeaders: true); diff --git a/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpContext.cs b/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpContext.cs index 1866967527..15c69432ed 100644 --- a/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpContext.cs +++ b/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpContext.cs @@ -28,7 +28,6 @@ namespace Microsoft.AspNetCore.Server.IIS.Core private const int PauseWriterThreshold = 65536; private const int ResumeWriterTheshold = PauseWriterThreshold / 2; - protected readonly IntPtr _pInProcessHandler; private readonly IISServerOptions _options; @@ -38,8 +37,8 @@ namespace Microsoft.AspNetCore.Server.IIS.Core private int _statusCode; private string _reasonPhrase; - private readonly object _onStartingSync = new object(); - private readonly object _onCompletedSync = new object(); + // Used to synchronize callback registration and native method calls + private readonly object _contextLock = new object(); protected Stack, object>> _onStarting; protected Stack, object>> _onCompleted; @@ -241,7 +240,7 @@ namespace Microsoft.AspNetCore.Server.IIS.Core // If at this point request was not upgraded just start a normal IO engine if (AsyncIO == null) { - AsyncIO = new AsyncIOEngine(_pInProcessHandler); + AsyncIO = new AsyncIOEngine(_contextLock, _pInProcessHandler); } } @@ -341,7 +340,7 @@ namespace Microsoft.AspNetCore.Server.IIS.Core public void OnStarting(Func callback, object state) { - lock (_onStartingSync) + lock (_contextLock) { if (HasResponseStarted) { @@ -358,7 +357,7 @@ namespace Microsoft.AspNetCore.Server.IIS.Core public void OnCompleted(Func callback, object state) { - lock (_onCompletedSync) + lock (_contextLock) { if (_onCompleted == null) { @@ -371,7 +370,7 @@ namespace Microsoft.AspNetCore.Server.IIS.Core protected async Task FireOnStarting() { Stack, object>> onStarting = null; - lock (_onStartingSync) + lock (_contextLock) { onStarting = _onStarting; _onStarting = null; @@ -395,7 +394,7 @@ namespace Microsoft.AspNetCore.Server.IIS.Core protected async Task FireOnCompleted() { Stack, object>> onCompleted = null; - lock (_onCompletedSync) + lock (_contextLock) { onCompleted = _onCompleted; _onCompleted = null; @@ -438,11 +437,6 @@ namespace Microsoft.AspNetCore.Server.IIS.Core NativeMethods.HttpPostCompletion(_pInProcessHandler, 0); } - public void IndicateCompletion(NativeMethods.REQUEST_NOTIFICATION_STATUS notificationStatus) - { - NativeMethods.HttpIndicateCompletion(_pInProcessHandler, notificationStatus); - } - internal void OnAsyncCompletion(int hr, int bytes) { AsyncIO.NotifyCompletion(hr, bytes); diff --git a/src/Microsoft.AspNetCore.Server.IIS/Core/IO/AsyncIOEngine.cs b/src/Microsoft.AspNetCore.Server.IIS/Core/IO/AsyncIOEngine.cs index 614367255a..e7a01b331d 100644 --- a/src/Microsoft.AspNetCore.Server.IIS/Core/IO/AsyncIOEngine.cs +++ b/src/Microsoft.AspNetCore.Server.IIS/Core/IO/AsyncIOEngine.cs @@ -12,6 +12,7 @@ namespace Microsoft.AspNetCore.Server.IIS.Core.IO { internal partial class AsyncIOEngine : IAsyncIOEngine { + private readonly object _contextSync; private readonly IntPtr _handler; private bool _stopped; @@ -23,8 +24,9 @@ namespace Microsoft.AspNetCore.Server.IIS.Core.IO private AsyncWriteOperation _cachedAsyncWriteOperation; private AsyncFlushOperation _cachedAsyncFlushOperation; - public AsyncIOEngine(IntPtr handler) + public AsyncIOEngine(object contextSync, IntPtr handler) { + _contextSync = contextSync; _handler = handler; } @@ -46,7 +48,7 @@ namespace Microsoft.AspNetCore.Server.IIS.Core.IO private void Run(AsyncIOOperation ioOperation) { - lock (this) + lock (_contextSync) { if (_stopped) { @@ -99,7 +101,7 @@ namespace Microsoft.AspNetCore.Server.IIS.Core.IO AsyncIOOperation.AsyncContinuation continuation; AsyncIOOperation.AsyncContinuation? nextContinuation = null; - lock (this) + lock (_contextSync) { Debug.Assert(_runningOperation != null); @@ -135,7 +137,7 @@ namespace Microsoft.AspNetCore.Server.IIS.Core.IO public void Dispose() { - lock (this) + lock (_contextSync) { _stopped = true; NativeMethods.HttpTryCancelIO(_handler); diff --git a/src/Microsoft.AspNetCore.Server.IIS/Core/IO/WebSocketsAsyncIOEngine.cs b/src/Microsoft.AspNetCore.Server.IIS/Core/IO/WebSocketsAsyncIOEngine.cs index 0c04523cb0..38c737dbce 100644 --- a/src/Microsoft.AspNetCore.Server.IIS/Core/IO/WebSocketsAsyncIOEngine.cs +++ b/src/Microsoft.AspNetCore.Server.IIS/Core/IO/WebSocketsAsyncIOEngine.cs @@ -10,6 +10,8 @@ namespace Microsoft.AspNetCore.Server.IIS.Core.IO { internal partial class WebSocketsAsyncIOEngine: IAsyncIOEngine { + private readonly object _contextLock; + private readonly IntPtr _handler; private bool _isInitialized; @@ -22,55 +24,65 @@ namespace Microsoft.AspNetCore.Server.IIS.Core.IO private AsyncInitializeOperation _cachedAsyncInitializeOperation; - public WebSocketsAsyncIOEngine(IntPtr handler) + public WebSocketsAsyncIOEngine(object contextLock, IntPtr handler) { + _contextLock = contextLock; _handler = handler; } public ValueTask ReadAsync(Memory memory) { - ThrowIfNotInitialized(); + lock (_contextLock) + { + ThrowIfNotInitialized(); - var read = GetReadOperation(); - read.Initialize(_handler, memory); - read.Invoke(); - return new ValueTask(read, 0); + var read = GetReadOperation(); + read.Initialize(_handler, memory); + read.Invoke(); + return new ValueTask(read, 0); + } } public ValueTask WriteAsync(ReadOnlySequence data) { - ThrowIfNotInitialized(); + lock (_contextLock) + { + ThrowIfNotInitialized(); - var write = GetWriteOperation(); - write.Initialize(_handler, data); - write.Invoke(); - return new ValueTask(write, 0); + var write = GetWriteOperation(); + write.Initialize(_handler, data); + write.Invoke(); + return new ValueTask(write, 0); + } } public ValueTask FlushAsync() { - if (_isInitialized) + lock (_contextLock) { - return new ValueTask(Task.CompletedTask); + if (_isInitialized) + { + return new ValueTask(Task.CompletedTask); + } + + NativeMethods.HttpEnableWebsockets(_handler); + + var init = GetInitializeOperation(); + init.Initialize(_handler); + + var continuation = init.Invoke(); + + if (continuation != null) + { + _isInitialized = true; + } + else + { + _initializationFlush = init; + } + + return new ValueTask(init, 0); } - - NativeMethods.HttpEnableWebsockets(_handler); - - var init = GetInitializeOperation(); - init.Initialize(_handler); - - var continuation = init.Invoke(); - - if (continuation != null) - { - _isInitialized = true; - } - else - { - _initializationFlush = init; - } - - return new ValueTask(init, 0); } public void NotifyCompletion(int hr, int bytes) @@ -100,7 +112,10 @@ namespace Microsoft.AspNetCore.Server.IIS.Core.IO public void Dispose() { - NativeMethods.HttpTryCancelIO(_handler); + lock (_contextLock) + { + NativeMethods.HttpTryCancelIO(_handler); + } } private WebSocketReadOperation GetReadOperation() => diff --git a/src/Microsoft.AspNetCore.Server.IIS/NativeMethods.cs b/src/Microsoft.AspNetCore.Server.IIS/NativeMethods.cs index 55b8084e59..de7bcb0562 100644 --- a/src/Microsoft.AspNetCore.Server.IIS/NativeMethods.cs +++ b/src/Microsoft.AspNetCore.Server.IIS/NativeMethods.cs @@ -70,9 +70,6 @@ namespace Microsoft.AspNetCore.Server.IIS [DllImport(AspNetCoreModuleDll)] private static extern int http_stop_incoming_requests(IntPtr pInProcessApplication); - [DllImport(AspNetCoreModuleDll)] - private static extern unsafe HttpApiTypes.HTTP_RESPONSE_V2* http_get_raw_response(IntPtr pInProcessHandler); - [DllImport(AspNetCoreModuleDll, CharSet = CharSet.Ansi)] private static extern int http_set_response_status_code(IntPtr pInProcessHandler, ushort statusCode, string pszReason); @@ -138,10 +135,6 @@ namespace Microsoft.AspNetCore.Server.IIS Validate(http_set_completion_status(pInProcessHandler, rquestNotificationStatus)); } - public static void HttpIndicateCompletion(IntPtr pInProcessHandler, REQUEST_NOTIFICATION_STATUS notificationStatus) - { - http_indicate_completion(pInProcessHandler, notificationStatus); - } public static void HttpRegisterCallbacks(IntPtr pInProcessApplication, PFN_REQUEST_HANDLER requestCallback, PFN_SHUTDOWN_HANDLER shutdownCallback, PFN_ASYNC_COMPLETION asyncCallback, IntPtr pvRequestContext, IntPtr pvShutdownContext) { Validate(register_callbacks(pInProcessApplication, requestCallback, shutdownCallback, asyncCallback, pvRequestContext, pvShutdownContext)); @@ -156,6 +149,7 @@ namespace Microsoft.AspNetCore.Server.IIS { return http_flush_response_bytes(pInProcessHandler, out fCompletionExpected); } + public static unsafe HttpApiTypes.HTTP_REQUEST_V2* HttpGetRawRequest(IntPtr pInProcessHandler) { return http_get_raw_request(pInProcessHandler); @@ -171,11 +165,6 @@ namespace Microsoft.AspNetCore.Server.IIS Validate(http_stop_incoming_requests(pInProcessApplication)); } - public static unsafe HttpApiTypes.HTTP_RESPONSE_V2* HttpGetRawResponse(IntPtr pInProcessHandler) - { - return http_get_raw_response(pInProcessHandler); - } - public static void HttpSetResponseStatusCode(IntPtr pInProcessHandler, ushort statusCode, string pszReason) { Validate(http_set_response_status_code(pInProcessHandler, statusCode, pszReason)); diff --git a/test/IISIntegration.FunctionalTests/Inprocess/ServerVariablesTest.cs b/test/IISIntegration.FunctionalTests/Inprocess/ServerVariablesTest.cs index 56b04c208e..020ef00213 100644 --- a/test/IISIntegration.FunctionalTests/Inprocess/ServerVariablesTest.cs +++ b/test/IISIntegration.FunctionalTests/Inprocess/ServerVariablesTest.cs @@ -1,6 +1,8 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System.Collections.Generic; +using System.Net.Http; using System.Threading.Tasks; using Microsoft.AspNetCore.Server.IntegrationTesting; using Microsoft.AspNetCore.Testing.xunit; @@ -38,5 +40,29 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests { Assert.DoesNotContain(@"\\?\", await _fixture.Client.GetStringAsync("/BasePath")); } + + [ConditionalFact] + public async Task GetServerVariableDoesNotCrash() + { + async Task RunRequests() + { + var client = new HttpClient() { BaseAddress = _fixture.Client.BaseAddress }; + + for (int j = 0; j < 10; j++) + { + var response = await client.GetStringAsync("/GetServerVariableStress"); + Assert.StartsWith("Response Begin", response); + Assert.EndsWith("Response End", response); + } + } + + List tasks = new List(); + for (int i = 0; i < 10; i++) + { + tasks.Add(Task.Run(RunRequests)); + } + + await Task.WhenAll(tasks); + } } } diff --git a/test/WebSites/InProcessWebSite/Startup.cs b/test/WebSites/InProcessWebSite/Startup.cs index 3aaba9b44f..2bf89384f8 100644 --- a/test/WebSites/InProcessWebSite/Startup.cs +++ b/test/WebSites/InProcessWebSite/Startup.cs @@ -738,5 +738,21 @@ namespace IISTestSite ctx.RequestServices.GetService().StopApplication(); }); } + + private async Task GetServerVariableStress(HttpContext context) + { + // This test simulates the scenario where native Flush call is being + // executed on background thread while request thread calls GetServerVariable + // concurrent native calls may cause native object corruption + + await context.Response.WriteAsync("Response Begin"); + for (int i = 0; i < 1000; i++) + { + await context.Response.WriteAsync(context.GetIISServerVariable("REMOTE_PORT")); + await context.Response.Body.FlushAsync(); + } + await context.Response.WriteAsync("Response End"); + } + } }