From 60a559719f1c813693f0dd7c82f113c5014ddb37 Mon Sep 17 00:00:00 2001 From: Mike Harder Date: Mon, 6 Aug 2018 14:41:11 -0700 Subject: [PATCH] Reduce probability of startup port collisions (#1136) - GetTickCount() is limited to the resolution of the system timer, which is typically 10-16 ms. If two apps in separate app pools are started within this time window, it's possible GetTickCount() will return the same value, which causes the apps to try the same random port(s). - Addresses #1124 --- .../AspNetCore/Inc/serverprocess.h | 6 +- .../AspNetCore/src/serverprocess.cxx | 10 +-- .../serverprocess.cpp | 10 +-- .../serverprocess.h | 4 ++ .../Utilities/FunctionalTestsBase.cs | 8 ++- test/Common.Tests/Utilities/DisposableList.cs | 25 +++++++ .../OutOfProcess/MultipleAppTests.cs | 66 +++++++++++++++++++ 7 files changed, 118 insertions(+), 11 deletions(-) create mode 100644 test/Common.Tests/Utilities/DisposableList.cs create mode 100644 test/IISExpress.FunctionalTests/OutOfProcess/MultipleAppTests.cs diff --git a/src/AspNetCoreModuleV1/AspNetCore/Inc/serverprocess.h b/src/AspNetCoreModuleV1/AspNetCore/Inc/serverprocess.h index 0a721c1544..94b65bad54 100644 --- a/src/AspNetCoreModuleV1/AspNetCore/Inc/serverprocess.h +++ b/src/AspNetCoreModuleV1/AspNetCore/Inc/serverprocess.h @@ -3,6 +3,8 @@ #pragma once +#include + #define MIN_PORT 1025 #define MAX_PORT 48000 #define MAX_RETRY 10 @@ -303,6 +305,8 @@ private: volatile BOOL m_fReady; mutable LONG m_cRefs; + std::mt19937 m_randomGenerator; + DWORD m_dwPort; DWORD m_dwStartupTimeLimitInMS; DWORD m_dwShutdownTimeLimitInMS; @@ -331,4 +335,4 @@ private: PROCESS_MANAGER *m_pProcessManager; ENVIRONMENT_VAR_HASH *m_pEnvironmentVarTable ; -}; \ No newline at end of file +}; diff --git a/src/AspNetCoreModuleV1/AspNetCore/src/serverprocess.cxx b/src/AspNetCoreModuleV1/AspNetCore/src/serverprocess.cxx index 8b5e6f47e2..4caf8cd3a3 100644 --- a/src/AspNetCoreModuleV1/AspNetCore/src/serverprocess.cxx +++ b/src/AspNetCoreModuleV1/AspNetCore/src/serverprocess.cxx @@ -89,13 +89,15 @@ SERVER_PROCESS::GetRandomPort BOOL fPortInUse = FALSE; DWORD dwActualProcessId = 0; + std::uniform_int_distribution<> dist(MIN_PORT, MAX_PORT); + if (g_fNsiApiNotSupported) { // // the default value for optional parameter dwExcludedPort is 0 which is reserved // a random number between MIN_PORT and MAX_PORT // - while ((*pdwPickedPort = (rand() % (MAX_PORT - MIN_PORT)) + MIN_PORT + 1) == dwExcludedPort); + while ((*pdwPickedPort = dist(m_randomGenerator)) == dwExcludedPort); } else { @@ -107,7 +109,7 @@ SERVER_PROCESS::GetRandomPort // determing whether the randomly generated port is // in use by any other process. // - while ((*pdwPickedPort = (rand() % (MAX_PORT - MIN_PORT)) + MIN_PORT + 1) == dwExcludedPort); + while ((*pdwPickedPort = dist(m_randomGenerator)) == dwExcludedPort); hr = CheckIfServerIsUp(*pdwPickedPort, &dwActualProcessId, &fPortInUse); } while (fPortInUse && ++cRetry < MAX_RETRY); @@ -1909,10 +1911,10 @@ SERVER_PROCESS::SERVER_PROCESS() : m_pForwarderConnection( NULL ), m_dwListeningProcessId( 0 ), m_hListeningProcessHandle( NULL ), - m_hShutdownHandle( NULL ) + m_hShutdownHandle( NULL ), + m_randomGenerator( std::random_device()() ) { InterlockedIncrement(&g_dwActiveServerProcesses); - srand(GetTickCount()); for(INT i=0;i dist(MIN_PORT, MAX_PORT); + if (g_fNsiApiNotSupported) { // // the default value for optional parameter dwExcludedPort is 0 which is reserved // a random number between MIN_PORT and MAX_PORT // - while ((*pdwPickedPort = (rand() % (MAX_PORT - MIN_PORT)) + MIN_PORT + 1) == dwExcludedPort); + while ((*pdwPickedPort = dist(m_randomGenerator)) == dwExcludedPort); } else { @@ -125,7 +127,7 @@ SERVER_PROCESS::GetRandomPort // determing whether the randomly generated port is // in use by any other process. // - while ((*pdwPickedPort = (rand() % (MAX_PORT - MIN_PORT)) + MIN_PORT + 1) == dwExcludedPort); + while ((*pdwPickedPort = dist(m_randomGenerator)) == dwExcludedPort); hr = CheckIfServerIsUp(*pdwPickedPort, &dwActualProcessId, &fPortInUse); } while (fPortInUse && ++cRetry < MAX_RETRY); @@ -1705,10 +1707,10 @@ SERVER_PROCESS::SERVER_PROCESS() : m_pForwarderConnection(NULL), m_dwListeningProcessId(0), m_hListeningProcessHandle(NULL), - m_hShutdownHandle(NULL) + m_hShutdownHandle(NULL), + m_randomGenerator(std::random_device()()) { //InterlockedIncrement(&g_dwActiveServerProcesses); - srand(GetTickCount()); for (INT i=0; i + #define MIN_PORT 1025 #define MAX_PORT 48000 #define MAX_RETRY 10 @@ -257,6 +259,8 @@ private: volatile BOOL m_fReady; mutable LONG m_cRefs; + std::mt19937 m_randomGenerator; + DWORD m_dwPort; DWORD m_dwStartupTimeLimitInMS; DWORD m_dwShutdownTimeLimitInMS; diff --git a/test/Common.FunctionalTests/Utilities/FunctionalTestsBase.cs b/test/Common.FunctionalTests/Utilities/FunctionalTestsBase.cs index 171bbbf22f..108e8ca84d 100644 --- a/test/Common.FunctionalTests/Utilities/FunctionalTestsBase.cs +++ b/test/Common.FunctionalTests/Utilities/FunctionalTestsBase.cs @@ -20,7 +20,7 @@ namespace Microsoft.AspNetCore.Server.IntegrationTesting protected ApplicationDeployer _deployer; - protected virtual async Task DeployAsync(IISDeploymentParameters parameters) + protected ApplicationDeployer CreateDeployer(IISDeploymentParameters parameters) { if (!parameters.EnvironmentVariables.ContainsKey(DebugEnvironmentVariable)) { @@ -37,8 +37,12 @@ namespace Microsoft.AspNetCore.Server.IntegrationTesting throw new InvalidOperationException("All tests should use ApplicationPublisher"); } - _deployer = IISApplicationDeployerFactory.Create(parameters, LoggerFactory); + return IISApplicationDeployerFactory.Create(parameters, LoggerFactory); + } + protected virtual async Task DeployAsync(IISDeploymentParameters parameters) + { + _deployer = CreateDeployer(parameters); return (IISDeploymentResult)await _deployer.DeployAsync(); } diff --git a/test/Common.Tests/Utilities/DisposableList.cs b/test/Common.Tests/Utilities/DisposableList.cs new file mode 100644 index 0000000000..78f76e41c2 --- /dev/null +++ b/test/Common.Tests/Utilities/DisposableList.cs @@ -0,0 +1,25 @@ +// 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; +using System.Collections.Generic; + +namespace Microsoft.AspNetCore.Server.IntegrationTesting +{ + public class DisposableList : List, IDisposable where T : IDisposable + { + public DisposableList() : base() { } + + public DisposableList(IEnumerable collection) : base(collection) { } + + public DisposableList(int capacity) : base(capacity) { } + + public void Dispose() + { + foreach (var item in this) + { + item?.Dispose(); + } + } + } +} diff --git a/test/IISExpress.FunctionalTests/OutOfProcess/MultipleAppTests.cs b/test/IISExpress.FunctionalTests/OutOfProcess/MultipleAppTests.cs new file mode 100644 index 0000000000..cf647f46e7 --- /dev/null +++ b/test/IISExpress.FunctionalTests/OutOfProcess/MultipleAppTests.cs @@ -0,0 +1,66 @@ +// 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; +using System.Net.Http; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Server.IIS.FunctionalTests.Utilities; +using Microsoft.AspNetCore.Server.IntegrationTesting; +using Microsoft.AspNetCore.Testing.xunit; +using Xunit; + +namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests +{ + [Collection(PublishedSitesCollection.Name)] + public class MultipleAppTests : IISFunctionalTestBase + { + private readonly PublishedSitesFixture _fixture; + + public MultipleAppTests(PublishedSitesFixture fixture) + { + _fixture = fixture; + } + + [ConditionalTheory] + [InlineData(AncmVersion.AspNetCoreModule)] + [InlineData(AncmVersion.AspNetCoreModuleV2)] + public async Task Startup(AncmVersion ancmVersion) + { + const int numApps = 10; + + using (var deployers = new DisposableList()) + { + var deploymentResults = new List(); + + // Deploy all apps + for (var i = 0; i < numApps; i++) + { + var deploymentParameters = _fixture.GetBaseDeploymentParameters(hostingModel: IntegrationTesting.HostingModel.OutOfProcess); + deploymentParameters.AncmVersion = ancmVersion; + + var deployer = CreateDeployer(deploymentParameters); + deployers.Add(deployer); + deploymentResults.Add(await deployer.DeployAsync()); + } + + // Start all requests as quickly as possible, so apps are started as quickly as possible, + // to test possible race conditions when multiple apps start at the same time. + var requestTasks = new List>(); + foreach (var deploymentResult in deploymentResults) + { + requestTasks.Add(deploymentResult.HttpClient.GetAsync("/HelloWorld")); + } + + // Verify all apps started and return expected response + foreach (var requestTask in requestTasks) + { + var response = await requestTask; + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var responseText = await response.Content.ReadAsStringAsync(); + Assert.Equal("Hello World", responseText); + } + } + } + } +}