Support attaching to an existing request queue in HTTP.SYS #14182

bkatms · 2019-09-20T07:48:05Z

Support attaching to an existing request queue in HTTP.SYS ( #5866). This is an addendum to
“https://github.com/aspnet/AspNetCore/issues/13461”.

This is criticial (P0) functionality required by Office 365 in support of our new ASP.Net Core 3.X architecture for data access to Substrate Storage. This is the storage that is mission critical for Office 365.

dnfclas · 2019-09-20T07:48:24Z

All CLA requirements met.

Tratcher

Thanks, getting this rebased does help.

There are some open issues from #5866 that we hadn't figured out how to address yet. Most of these are centered around ensuring each listener on a queue is using the same configuration. For things like timeouts it's at most a warning that the config is out of sync, but for auth and PathBase it can affect how requests are processed.

PathBase will need a fallback mode when attached to existing queues where it can't look things up by index.
For auth we might want to query the queue settings to see if they match and fail otherwise
I'm not sure if we can query for the timeout settings.
Some options can be changed after the server starts such as UrlPrefixes. We need to make sure those code paths are disabled (and throw?) if this instance did not create the queue.
Do we need to surface an API that says if this instance did attach or create?

I'll pull your branch and see what I can work on in your off hours. Want to give me push access to your fork?

src/Servers/HttpSys/src/NativeInterop/RequestQueue.cs

src/Servers/HttpSys/ref/Microsoft.AspNetCore.Server.HttpSys.netcoreapp.cs

src/Servers/HttpSys/src/NativeInterop/RequestQueue.cs

src/Servers/HttpSys/test/FunctionalTests/ServerTests.cs

src/Servers/HttpSys/src/NativeInterop/RequestQueue.cs

src/Servers/HttpSys/src/HttpSysListener.cs

analogrelay · 2019-09-20T14:51:31Z

FYI @Tratcher check in with me before merging. As this is feature work we'll require special approval for 3.1 (shouldn't be a problem).

Tratcher · 2019-09-20T23:08:28Z

Didn't make any progress on this today other than squashing and relocating my old branch to make it easier to reference: 2c48243

Tratcher · 2019-09-23T23:01:07Z

Made some progress, I pulled in the missing parts from the prior branch including tests and some misc fixes. I'll start on the actual review tomorrow.

src/Servers/HttpSys/src/RequestProcessing/Request.cs

Tratcher · 2019-09-27T16:51:00Z

I think that's everything for now.

I've dealt with PathBase. It can't query prefixes configured elsewhere but can do a match against ones you provide in the app.
For auth you must make sure your schemes are in sync or it's not going to work well. We can discuss guardrails in the future if people have issues.
Many settings are marked as not applicable in Attach mode.
If using CreateOrAttach, there is a log statement indicating if it attached but there's no API to show the results in the app. This isn't a priority for 3.1 since our driving customer doesn't plan to use CreateOrAttach. In the future consider surfacing this as a server feature? I'm not sure what scenarios would require knowing the difference (HotAdd maybe?).
Suppressed default urls when attached to an existing queue. This is going to case a 5.x merge conflict because Justin recently refactored that code path in master.

@halter73 @bkatms please review.

analogrelay · 2019-09-27T16:55:34Z

paging @davidfowl @rynowak for API review (not sure who else wants to be involved or if a GitHub team has been set up yet).

src/Servers/HttpSys/src/RequestQueueMode.cs

src/Servers/HttpSys/src/UrlPrefixCollection.cs

bkatms · 2019-09-27T20:03:28Z

The changes look fine to me. Thank you.

bkatms · 2019-09-27T20:25:14Z

Note relative to "Created" - my understanding is that HTTP.SYS will automatically delete a queue if there are no active references to it by any process.

In our case we will have a management process create and configure the queues and pass then name into our ASP.NET Core process - this is similar to how IIS works when it starts W3WP.exe. We will either directly spawn (security context inheritance) or set the ACLs appropriately - TBD. In any case we plan on handling that part.

Let me know when you have a drop for me to begin testing on.

Cheers!

jkotalik · 2019-09-30T18:36:33Z

src/Servers/HttpSys/src/AuthenticationManager.cs

@@ -32,6 +32,9 @@ internal AuthenticationManager()
        {
        }

+        /// <summary>
+        /// When attaching to an existing queue this setting must match the one used to create the queue.


This should probably be a remark rather than summary.

Remarks aren't visible in most situations.

src/Servers/HttpSys/src/HttpSysOptions.cs

jkotalik · 2019-09-30T18:49:46Z

src/Servers/HttpSys/src/NativeInterop/RequestQueue.cs

+                        out requestQueueHandle);
+            }
+
+            if (flags == HttpApiTypes.HTTP_CREATE_REQUEST_QUEUE_FLAG.OpenExisting && statusCode == UnsafeNclNativeMethods.ErrorCodes.ERROR_FILE_NOT_FOUND)


Shouldn't this technically check CreateOrAttach and OpenExisting?

That's why I'm checking the flags and not the RequestQueueMode enum.

Edit: Nevermind, I answered the wrong question. No, you want this detailed error for both Attach and CreateOrAttach. You tried to attach to a thing by name and it wasn't there. For CreateOrAttach that should only happen with a strange race condition.

For CreateOrAttach that should only happen with a strange race condition.

Yeah, the strange race condition is what I was referring to.

I don't think it's really worth guarding against that race condition, since to do so would require some sort of looping.

src/Servers/HttpSys/src/NativeInterop/RequestQueue.cs

src/Servers/HttpSys/src/UrlPrefix.cs

src/Servers/HttpSys/src/UrlPrefixCollection.cs

src/Servers/HttpSys/test/FunctionalTests/Listener/ServerOnExistingQueueTests.cs

# Conflicts: # src/Servers/HttpSys/ref/Microsoft.AspNetCore.Server.HttpSys.netcoreapp.cs

analogrelay · 2019-10-01T16:17:04Z

@davidfowl this needs to be merged Very Soon. What if anything do you want to do for API review here?

davidfowl · 2019-10-01T17:06:54Z

For auth you must make sure your schemes are in sync or it's not going to work well. We can discuss guardrails in the future if people have issues.

Many settings are marked as not applicable in Attach mode.

What’s the failure mode here?

Tratcher · 2019-10-01T17:15:04Z

For auth you must make sure your schemes are in sync or it's not going to work well. We can discuss guardrails in the future if people have issues.

Many settings are marked as not applicable in Attach mode.

What’s the failure mode here?

The managed layer enforces AllowAnonymous and adds challenges based on what's configured. Worst case you end up requiring more auth than the underlying queue supports and the client gets rejected with 401s.

bkatms · 2019-10-01T17:17:59Z

I am good here.

src/Servers/HttpSys/src/NativeInterop/RequestQueue.cs

Tratcher · 2019-10-01T21:57:35Z

@anurse this is ready to merge.

bkatms requested review from analogrelay, halter73, jkotalik and Tratcher as code owners September 20, 2019 07:48

Tratcher reviewed Sep 20, 2019

View reviewed changes

analogrelay added this to the 3.1.0-preview1 milestone Sep 20, 2019

analogrelay added the area-servers label Sep 20, 2019

Tratcher changed the title ~~#5866~~ Support attaching to an existing request queue in HTTP.SYS Sep 20, 2019

Tratcher self-assigned this Sep 20, 2019

davidfowl added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Sep 24, 2019

Tratcher force-pushed the bkress/httpsysexistingq branch from 27a8b65 to bf83d2c Compare September 24, 2019 19:53

Tratcher reviewed Sep 24, 2019

View reviewed changes

src/Servers/HttpSys/src/RequestProcessing/Request.cs Outdated Show resolved Hide resolved

davidfowl added the API Review label Sep 27, 2019

davidfowl reviewed Sep 27, 2019

View reviewed changes

src/Servers/HttpSys/src/RequestQueueMode.cs Show resolved Hide resolved

davidfowl reviewed Sep 27, 2019

View reviewed changes

src/Servers/HttpSys/src/UrlPrefixCollection.cs Show resolved Hide resolved

jkotalik reviewed Sep 30, 2019

View reviewed changes

bkatms and others added 6 commits October 1, 2019 08:26

Initial port of old branch for existing queues dotnet#5866

5495097

Ref assemblies

ceaa798

# Conflicts: # src/Servers/HttpSys/ref/Microsoft.AspNetCore.Server.HttpSys.netcoreapp.cs

Add sample app for manual testing

720ffaf

Pull in other parts of old PR (Some tests failing)

ff1c096

Fix path base, get tests passing

c2c39f8

Remove controller mode

cb93d46

Tratcher added 9 commits October 1, 2019 08:26

Explicit flags type

d811b19

Rename Mode property. Add comments about what's settings apply.

2bd891b

PathBase searching

eff24ed

Reorder CreateOrAttach

1a8efc4

Remove outdated test

f95bff3

Do not add a default address

5880415

PR Cleanup

4f58c29

Doc comments

8bab6b5

Longest match test

9dc1ec6

Tratcher force-pushed the bkress/httpsysexistingq branch from 8c8e0ad to 9dc1ec6 Compare October 1, 2019 15:26

Tratcher changed the base branch from release/3.1 to release/3.1-preview1 October 1, 2019 15:26

halter73 reviewed Oct 1, 2019

View reviewed changes

src/Servers/HttpSys/src/NativeInterop/RequestQueue.cs Outdated Show resolved Hide resolved

halter73 approved these changes Oct 1, 2019

View reviewed changes

jkotalik approved these changes Oct 1, 2019

View reviewed changes

Include queue name in log

f0806c2

analogrelay merged commit 51ae61b into dotnet:release/3.1-preview1 Oct 1, 2019

pranavkm removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Mar 22, 2021

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

Support attaching to an existing request queue in HTTP.SYS #14182

Support attaching to an existing request queue in HTTP.SYS #14182

Uh oh!

Conversation

bkatms commented Sep 20, 2019

Uh oh!

dnfclas commented Sep 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tratcher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

analogrelay commented Sep 20, 2019

Uh oh!

Tratcher commented Sep 20, 2019

Uh oh!

Tratcher commented Sep 23, 2019

Uh oh!

Uh oh!

Tratcher commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analogrelay commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkatms commented Sep 27, 2019

Uh oh!

bkatms commented Sep 27, 2019

Uh oh!

jkotalik Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

Tratcher Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkotalik Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

Tratcher Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotalik Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

halter73 Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

analogrelay commented Oct 1, 2019

Uh oh!

davidfowl commented Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tratcher commented Oct 1, 2019

Uh oh!

bkatms commented Oct 1, 2019 via email • edited by Tratcher Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Tratcher commented Oct 1, 2019

Uh oh!

Uh oh!

dnfclas commented Sep 20, 2019 •

edited

Loading

Tratcher commented Sep 27, 2019 •

edited

Loading

analogrelay commented Sep 27, 2019 •

edited

Loading

Tratcher Sep 30, 2019 •

edited

Loading

davidfowl commented Oct 1, 2019 •

edited

Loading

bkatms commented Oct 1, 2019 via email •

edited by Tratcher

Loading