Merge pull request #1015 from aspnet/pakrym/flush-sync

Sync native calls

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)
         {

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);
 

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();
+        // Used to synchronize callback registration and native method calls
-        private readonly object _onCompletedSync = new object();
+        private readonly object _contextLock = new object();
 
         protected Stack<KeyValuePair<Func<object, Task>, object>> _onStarting;
         protected Stack<KeyValuePair<Func<object, Task>, 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<object, Task> callback, object state)
         {
-            lock (_onStartingSync)
+            lock (_contextLock)
             {
                 if (HasResponseStarted)
                 {
@@ -358,7 +357,7 @@ namespace Microsoft.AspNetCore.Server.IIS.Core
 
         public void OnCompleted(Func<object, Task> 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<KeyValuePair<Func<object, Task>, 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<KeyValuePair<Func<object, Task>, 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);

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);

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<int> ReadAsync(Memory<byte> memory)
         {
-            ThrowIfNotInitialized();
+            lock (_contextLock)
+            {
+                ThrowIfNotInitialized();
 
-            var read = GetReadOperation();
+                var read = GetReadOperation();
-            read.Initialize(_handler, memory);
+                read.Initialize(_handler, memory);
-            read.Invoke();
+                read.Invoke();
-            return new ValueTask<int>(read, 0);
+                return new ValueTask<int>(read, 0);   
+            }
         }
 
         public ValueTask<int> WriteAsync(ReadOnlySequence<byte> data)
         {
-            ThrowIfNotInitialized();
+            lock (_contextLock)
+            {
+                ThrowIfNotInitialized();
 
-            var write = GetWriteOperation();
+                var write = GetWriteOperation();
-            write.Initialize(_handler, data);
+                write.Initialize(_handler, data);
-            write.Invoke();
+                write.Invoke();
-            return new ValueTask<int>(write, 0);
+                return new ValueTask<int>(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() =>

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));

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<Task> tasks = new List<Task>();
+            for (int i = 0; i < 10; i++)
+            {
+                tasks.Add(Task.Run(RunRequests));
+            }
+
+            await Task.WhenAll(tasks);
+        }
     }
 }

test/WebSites/InProcessWebSite/Startup.cs

@@ -738,5 +738,21 @@ namespace IISTestSite
                 ctx.RequestServices.GetService<IApplicationLifetime>().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");
+        }
+
     }
 }