-
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
base: main
Are you sure you want to change the base?
Fix IIS recycle regressions #59998
Conversation
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Fix local websocket tests.
<logFile logFormat="W3C" directory="%IIS_USER_HOME%\Logs" /> | ||
<traceFailedRequestsLogging directory="%IIS_USER_HOME%\TraceLogFiles" enabled="true" maxLogFileSizeKB="1024" /> | ||
<logFile logFormat="W3C" directory="%TEMP%\IISTests\Logs" /> | ||
<traceFailedRequestsLogging directory="%TEMP%\IISTests\TraceLogFiles" enabled="true" maxLogFileSizeKB="1024" /> |
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.
Write logs to temp instead of user directory. If you know, you know.
_wcsicmp(pwszChangePath, L"MACHINE") != 0 && | ||
_wcsicmp(pwszChangePath, L"MACHINE/WEBROOT") != 0) | ||
{ | ||
_wcsicmp(pwszChangePath, L"MACHINE/WEBROOT") > 0 && |
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.
If this is a prefix check shouldn't it be e.g.
_wcsnicmp(pwszChangePath, L"MACHINE/WEBROOT", wcslen(L"MACHINE/WEBROOT")) == 0
Why the >0 check?
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.
Also, I'm curious why the previous code looked like it did if it was in fact intending to do a prefix check.
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.
Also, I'm curious why the previous code looked like it did if it was in fact intending to do a prefix check.
That's fair, it is written oddly. I haven't seen any examples of a non "MACHINE/WEBROOT/..." value which is why I assumed it was a prefix check.
Your proposed code would be an exact check not a prefix check. The > 0
I'm using checks that it matches and contains more characters.
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.
Actually, the wcsicmp
you're using will return a positive value for anything that's lexicographically greater, so even MACHINE/ZZZ
will pass. It doesn't actually do a prefix check.
The wcsnicmp approach I suggested would be a prefix check since it also specifies the number of characters to compare.
@@ -82,15 +87,16 @@ ASPNET_CORE_GLOBAL_MODULE::OnGlobalConfigurationChange( | |||
|
|||
// Test for an error. |
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.
This comment could be better. Not sure what error it's talking about 😆
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.
I think it was referring to the null check.
Fixes #58939 and both issues mentioned in #54531, both the empty path issue described in the initial post and the bad shutdown mentioned in #54531 (comment)
These were regressed by #52807
The two regressions were:
disallowRotationOnConfigChange
wastrue
we would still shutdown the application on config changes.The third fix wasn't a regression but was simple enough to fix and part of one of the issues linked above so included a fix for it. Our usage of
_wcsicmp
didn't properly check the path started with"MACHINE/WEBROOT"
, it would pass for""
, or"MAC"
,"MACHINE/W"
, etc.We have the test
ConfigurationChangeCanBeIgnoredInProcess
which should have caught one of the regressions, but since it was testing for the lack of app shutdown it doesn't have any way to know that nothing happened, fixed the test to have a bit of a delay so it has a better chance of failing in the future if we regress this.We also have a multiple application test, but it was only running for IISExpress and it didn't have a restart scenario. Moved the test to the common tests so it's run by IIS as well and added a scenario where 1 of the applications restarts but the others don't.