Skip to content

Fix and test HttpSys delegation #36677

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

Merged
merged 7 commits into from
Sep 18, 2021

Conversation

Tratcher
Copy link
Member

Fixes #28396 (adding out-of-proc test)

When writing the new test we found that this feature regressed on Win11 because Http.Sys requires a new initialization flag for the feature detection API to work.

I also refactored the initialization logic so IsFeatureSupported is only called once at startup.

I plan to backport this to 6.0-rc2. We might also need to backport it to 5.0 if partners get blocked.

@Tratcher Tratcher force-pushed the tratcher/httpsys/delegate branch from d7b917f to c55fcb3 Compare September 17, 2021 21:03
@shirhatti
Copy link
Contributor

👀 @NGloreous

@@ -136,18 +137,17 @@ private static void InitHttpApi(ushort majorVersion, ushort minorVersion)
version.HttpApiMajorVersion = majorVersion;
version.HttpApiMinorVersion = minorVersion;

var statusCode = HttpInitialize(version, (uint)HTTP_FLAGS.HTTP_INITIALIZE_SERVER, null);
var statusCode = HttpInitialize(version, (uint)(HTTP_FLAGS.HTTP_INITIALIZE_SERVER | HTTP_FLAGS.HTTP_INITIALIZE_CONFIG), null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happen on older OSes that don't have this API?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTTP_INITIALIZE_CONFIG isn't a new flag, I think it's been around forever. What changed is that HttpIsFeatureSupported now requires it.

@shirhatti
Copy link
Contributor

shirhatti commented Sep 17, 2021

Looks like this doc needs to be updated. The functions listed that require HTTP_INITIALIZE_CONFIG don't include HttpIsFeatureSupported

EDIT: Sent a PR- https://github.com/MicrosoftDocs/sdk-api/pull/906/files

using Microsoft.AspNetCore.Testing;
using Xunit.Abstractions;

namespace Microsoft.AspNetCore.Server.HttpSys.NonHelixTests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why we don't want these to run on helix?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never figured out how to make them work there. #8247

@Tratcher Tratcher enabled auto-merge (squash) September 18, 2021 02:25
@Tratcher
Copy link
Member Author

/backport to release/6.0-rc2

@github-actions
Copy link
Contributor

Started backporting to release/6.0-rc2: https://github.com/dotnet/aspnetcore/actions/runs/1247644575

@github-actions
Copy link
Contributor

@Tratcher backporting to release/6.0-rc2 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Out of proc delegation tests
Applying: Troubleshoot IsFeatureSupported
Applying: Fix test
Applying: Fix formatting
Applying: Fix sln
Using index info to reconstruct a base tree...
M	AspNetCore.sln
Falling back to patching base and 3-way merge...
Auto-merging AspNetCore.sln
CONFLICT (content): Merge conflict in AspNetCore.sln
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 Fix sln
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@Tratcher Tratcher merged commit f72785f into dotnet:main Sep 18, 2021
@Tratcher Tratcher deleted the tratcher/httpsys/delegate branch September 18, 2021 02:53
@ghost ghost added this to the 7.0-preview1 milestone Sep 18, 2021
@amcasey amcasey added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add out-of-proc test for request queue delegation
4 participants