-
Notifications
You must be signed in to change notification settings - Fork 10.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix IIS recycle regressions #59998
Open
BrennanConroy
wants to merge
9
commits into
main
Choose a base branch
from
brecon/iisoutofprocessrecycle
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Fix IIS recycle regressions #59998
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
88c9048
whip
BrennanConroy e8c1033
fixuo
BrennanConroy 82bfbca
ignore config
BrennanConroy 4fddee1
Fixups
BrennanConroy 127907b
cleanup
BrennanConroy 8c6e8e5
using
BrennanConroy 2324b9b
using
BrennanConroy bb29868
just empty path check
BrennanConroy f7fbf14
fixup
BrennanConroy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,7 +51,7 @@ internal IISTestSiteFixture(Action<IISDeploymentParameters> configure) | |
//DeploymentParameters.EnvironmentVariables.Add("ASPNETCORE_MODULE_DEBUG", "console"); | ||
|
||
// This queue does not have websockets enabled currently, adding the module will break all tests using this fixture. | ||
if (!HelixHelper.GetTargetHelixQueue().ToLowerInvariant().Contains("windows.amd64.server2022")) | ||
if (!(HelixHelper.GetTargetHelixQueue() ?? "").ToLowerInvariant().Contains("windows.amd64.server2022")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix local websocket tests. |
||
{ | ||
DeploymentParameters.EnableModule("WebSocketModule", "%IIS_BIN%/iiswsock.dll"); | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
129 changes: 129 additions & 0 deletions
129
src/Servers/IIS/IIS/test/Common.FunctionalTests/MultipleAppTests.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,129 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Net; | ||
using System.Net.Http; | ||
using Microsoft.AspNetCore.Server.IIS.FunctionalTests.Utilities; | ||
using Microsoft.AspNetCore.Server.IntegrationTesting; | ||
using Microsoft.AspNetCore.InternalTesting; | ||
using System.Globalization; | ||
|
||
#if !IIS_FUNCTIONALS | ||
using Microsoft.AspNetCore.Server.IIS.FunctionalTests; | ||
|
||
#if IISEXPRESS_FUNCTIONALS | ||
namespace Microsoft.AspNetCore.Server.IIS.IISExpress.FunctionalTests; | ||
#elif NEWHANDLER_FUNCTIONALS | ||
namespace Microsoft.AspNetCore.Server.IIS.NewHandler.FunctionalTests; | ||
#elif NEWSHIM_FUNCTIONALS | ||
namespace Microsoft.AspNetCore.Server.IIS.NewShim.FunctionalTests; | ||
#endif | ||
|
||
#else | ||
namespace Microsoft.AspNetCore.Server.IIS.FunctionalTests; | ||
#endif | ||
|
||
[Collection(PublishedSitesCollection.Name)] | ||
[SkipOnHelix("Unsupported queue", Queues = "Windows.Amd64.VS2022.Pre.Open;")] | ||
public class MultipleAppTests : IISFunctionalTestBase | ||
{ | ||
public MultipleAppTests(PublishedSitesFixture fixture) : base(fixture) | ||
{ | ||
} | ||
|
||
[ConditionalFact] | ||
public async Task StartupManyAppsSuccessful() | ||
{ | ||
const int numApps = 10; | ||
|
||
using (var deployers = new DisposableList<ApplicationDeployer>()) | ||
{ | ||
var deploymentResults = new List<DeploymentResult>(); | ||
|
||
var deploymentParameters = Fixture.GetBaseDeploymentParameters(hostingModel: HostingModel.OutOfProcess); | ||
var deployer = CreateDeployer(deploymentParameters); | ||
deployers.Add(deployer); | ||
|
||
// Deploy all apps | ||
for (var i = 0; i < numApps; i++) | ||
{ | ||
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<Task<HttpResponseMessage>>(); | ||
foreach (var deploymentResult in deploymentResults) | ||
{ | ||
requestTasks.Add(deploymentResult.HttpClient.RetryRequestAsync("/HelloWorld", r => r.IsSuccessStatusCode)); | ||
} | ||
|
||
// 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); | ||
} | ||
} | ||
} | ||
|
||
[ConditionalFact] | ||
public async Task RestartAppShouldNotAffectOtherApps() | ||
{ | ||
const int numApps = 10; | ||
|
||
using (var deployers = new DisposableList<ApplicationDeployer>()) | ||
{ | ||
var deploymentResults = new List<DeploymentResult>(); | ||
|
||
var deploymentParameters = Fixture.GetBaseDeploymentParameters(hostingModel: HostingModel.OutOfProcess); | ||
var deployer = CreateDeployer(deploymentParameters); | ||
deployers.Add(deployer); | ||
|
||
// Deploy all apps | ||
for (var i = 0; i < numApps; i++) | ||
{ | ||
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<Task<HttpResponseMessage>>(); | ||
foreach (var deploymentResult in deploymentResults) | ||
{ | ||
requestTasks.Add(deploymentResult.HttpClient.RetryRequestAsync("/ProcessId", r => r.IsSuccessStatusCode)); | ||
} | ||
|
||
List<int> processIDs = new(); | ||
// 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(); | ||
processIDs.Add(int.Parse(responseText, CultureInfo.InvariantCulture)); | ||
} | ||
|
||
// Just "touching" web.config should be enough to restart the process | ||
deploymentResults[0].ModifyWebConfig(_ => { }); | ||
|
||
// Need to give time for process to start and finish restarting | ||
await deploymentResults[0].HttpClient.RetryRequestAsync("/ProcessId", | ||
async r => int.Parse(await r.Content.ReadAsStringAsync(), CultureInfo.InvariantCulture) != processIDs[0]); | ||
|
||
// First process should have restarted | ||
var res = await deploymentResults[0].HttpClient.RetryRequestAsync("/ProcessId", r => r.IsSuccessStatusCode); | ||
|
||
Assert.NotEqual(processIDs[0], int.Parse(await res.Content.ReadAsStringAsync(), CultureInfo.InvariantCulture)); | ||
|
||
// Every other process should stay the same | ||
for (var i = 1; i < deploymentResults.Count; i++) | ||
{ | ||
res = await deploymentResults[i].HttpClient.GetAsync("/ProcessId"); | ||
Assert.Equal(processIDs[i], int.Parse(await res.Content.ReadAsStringAsync(), CultureInfo.InvariantCulture)); | ||
} | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we loosening from strictly matching on
1
to any digit here? I see there are other updates related to multi-site handling in this PR. Does that address the regression or is it a new feature?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multi-site handling was added to add test coverage for the regression. Technically we might be able to pass in the site id to these methods so it's not using regex.