Use generated PInvokes and exchange types #50685

Tratcher · 2023-09-13T16:09:41Z

https://github.com/microsoft/cswin32 is a tool used to generate PInvoke interop code for Windows APIs. Http.Sys has a lot of hand rolled interop code that can be replaced with generated code. This should help reduce maintenance, improve correctness, and develop new features.

Open questions:

Are there any concerns with using this new build time dependency? Will says This-Is-Fine.
Source-build shenanigans:
- Source-build can't restore or use this new dependency
- IIS and HttpSys won't compile without it
- The default WebHost calls UseIIS
- The shared framework won't build without IIS and HttpSys.
- I conditionally removed all the code from IIS and HttpSys except a stub referenced by WebHost.
Can we clean up more of the code?
How do we convert the NativeOverlapped methods? My attempts all AV.

Dll size:

Before: 253kb
After: 252kb

wtgodbe

Approving adding the new dependency, shouldn't be any problems there

Tratcher · 2023-09-14T17:37:28Z

@wtgodbe thoughts on why this failed source build? Should that apply here?

eng/Dependencies.props

joperezr · 2023-09-14T23:01:34Z

Tagging @MichaelSimons for the new dependency question to make sure this is also not an issue for source-build. Michael, we don't think this should require a new prebuilt as this is only for windows, but I wanted to check with you anyway.

src/Shared/HttpSys/RequestProcessing/NativeRequestContext.cs

src/Servers/IIS/IIS/src/Microsoft.AspNetCore.Server.IIS.csproj

danmoseley · 2023-09-27T23:02:52Z

what's this doing under the covers -- is it a library, or a source generator?
also, curious what causes the size increase?

Tratcher · 2023-09-27T23:48:24Z

@danmoseley

what's this doing under the covers -- is it a library, or a source generator?

It's a source generator, it's creating the pinvoke definitions for us and embedding them in our dll.

also, curious what causes the size increase?

It used to be a much bigger difference when I was pulling in all of the Http.Sys types, that's why I called it out. I've scoped it down to only the types we actually use and the difference is negligible. In fact, after my latest changes the dll is now smaller than the original.

adityamandaleeka · 2023-09-28T20:31:07Z

cc: @AaronRobinsonMSFT

AaronRobinsonMSFT · 2023-09-29T04:10:25Z

/cc @jkoritzinsky

ghost · 2023-10-11T03:00:33Z

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

AaronRobinsonMSFT · 2023-10-11T13:49:42Z

This should help reduce maintenance, improve correctness, and develop new features.

Is this an area of active development? If so, then this is true otherwise this sort of change doesn't tend to add a lot of value.

How do we convert the NativeOverlapped methods? My attempts all AV.

If you can share what you've tried the interop team can help root out the issue.

Tratcher · 2023-10-11T16:03:07Z

Is this an area of active development? If so, then this is true otherwise this sort of change doesn't tend to add a lot of value.

Yes, we have partner teams request/contribute new features here regularly.

How do we convert the NativeOverlapped methods? My attempts all AV.

If you can share what you've tried the interop team can help root out the issue.

Any attempt I've made to call any generated APIs that take a NativeOverlapped do not work. I suspect part of the problem is that in most cases they take a NativeOverlapped and I am starting with a *NativeOverlapped or a SafeNativeOverlapped and there's some issue with the conversion. A live demo might be best, I'll ping you.

For example, when I try HttpReceiveHttpRequest:

// internal static unsafe uint HttpReceiveHttpRequest(SafeHandle RequestQueueHandle, ulong RequestId, winmdroot.Networking.HttpServer.HTTP_RECEIVE_HTTP_REQUEST_FLAGS Flags, out winmdroot.Networking.HttpServer.HTTP_REQUEST_V2 RequestBuffer, uint RequestBufferLength, uint* BytesReturned, global::System.Threading.NativeOverlapped? Overlapped)

// Vs what I have defined locally:
// internal static unsafe partial uint HttpReceiveHttpRequest(SafeHandle requestQueueHandle, ulong requestId, uint flags, Windows.Win32.Networking.HttpServer.HTTP_REQUEST_V1* pRequestBuffer, uint requestBufferLength, uint* pBytesReturned, NativeOverlapped* pOverlapped);

            var request = _requestContext.NativeRequestV2;
            retry = false;
            uint bytesTransferred = 0;
            statusCode = Windows.Win32.PInvoke.HttpReceiveHttpRequest(
                Server.RequestQueue.Handle,
                _requestContext.RequestId,
                // Small perf impact by not using HTTP_RECEIVE_REQUEST_FLAG_COPY_BODY
                // if the request sends header+body in a single TCP packet
                0u,
                out *request,
                _requestContext.Size,
                &bytesTransferred,
                *_overlapped);

System.NullReferenceException
  HResult=0x80004003
  Message=Object reference not set to an instance of an object.
  Source=System.Private.CoreLib
  StackTrace:
   at System.Threading.IOCompletionCallbackHelper.PerformSingleIOCompletionCallback(UInt32 errorCode, UInt32 numBytes, NativeOverlapped* pNativeOverlapped) in System.Threading\IOCompletionCallbackHelper.cs:line 32
   at System.Threading.PortableThreadPool.IOCompletionPoller.Callback.Invoke(Event e) in System.Threading\PortableThreadPool.cs:line 666
   at System.Threading.ThreadPoolTypedWorkItemQueue`2.System.Threading.IThreadPoolWorkItem.Execute() in System.Threading\ThreadPoolTypedWorkItemQueue.cs:line 44
   at System.Threading.ThreadPoolWorkQueue.Dispatch() in System.Threading\ThreadPoolWorkQueue.cs:line 690
   at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart() in System.Threading\PortableThreadPool.cs:line 1294

		Overlapped overlappedFromNative = Overlapped.GetOverlappedFromNative(pNativeOverlapped);
		object callback = overlappedFromNative._callback; // overlappedFromNative is null

Tratcher · 2023-10-11T19:02:51Z

Seems to be an issue with the code generator: microsoft/CsWin32#1066

Tratcher · 2023-10-20T20:55:25Z

/azp run

azure-pipelines · 2023-10-20T20:55:44Z

Azure Pipelines successfully started running 3 pipeline(s).

Tratcher · 2023-10-20T20:56:56Z

@adityamandaleeka thoughts on merging this? The NativeOverlapped issue isn't blocking and might take some time to resolve.

ghost · 2023-10-28T03:00:34Z

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

Tratcher · 2023-10-30T16:20:06Z

/azp run

azure-pipelines · 2023-10-30T16:20:30Z

Azure Pipelines successfully started running 3 pipeline(s).

BrennanConroy

Are there details about where the native overlapped issue shows up and what our reaction will be when the issue is fixed?

src/Servers/HttpSys/src/Microsoft.AspNetCore.Server.HttpSys.csproj

BrennanConroy · 2023-11-03T17:09:41Z

src/Servers/HttpSys/src/AsyncAcceptContext.cs

@@ -117,14 +116,14 @@ private uint QueueBeginGetContext()
                _requestContext.RequestId,
                // Small perf impact by not using HTTP_RECEIVE_REQUEST_FLAG_COPY_BODY
                // if the request sends header+body in a single TCP packet
-                (uint)HttpApiTypes.HTTP_FLAGS.NONE,
+                0u,


Feels like a small loss in code quality. Is there not a "none" flag in the httpsys headers?

No, there isn't. There isn't one defined natively either.
https://learn.microsoft.com/en-us/windows/win32/api/http/nf-http-httpreceivehttprequest

Value Meaning

0 (zero) Only the request headers are retrieved; the entity body is not copied.

HTTP_RECEIVE_REQUEST_FLAG_COPY_BODY The available entity body is copied along with the request headers. The pEntityChunks member of the HTTP_REQUEST structure points to the entity body.

HTTP_RECEIVE_REQUEST_FLAG_FLUSH_BODY All of the entity bodies are copied along with the request headers. The pEntityChunks member of the HTTP_REQUEST structure points to the entity body.

src/Servers/HttpSys/src/NativeMethods.txt

BrennanConroy · 2023-11-03T17:31:40Z

src/Servers/HttpSys/src/RequestProcessing/ResponseBody.cs

            if (!started)
            {
                statusCode = _requestContext.Response.SendHeaders(ref allocator, dataChunks, null, flags, false);
            }
            else
            {
-                fixed (HttpApiTypes.HTTP_DATA_CHUNK* pDataChunks = dataChunks)


This is because dataChunks comes from the unmanaged heap right? We just forgot to remove fixed

No, the fixed is now part of the generated code.

BrennanConroy · 2023-11-03T17:38:51Z

src/Servers/IIS/IIS/src/NativeMethods.cs

@@ -91,9 +92,10 @@ public enum REQUEST_NOTIFICATION_STATUS
    private static partial int http_get_application_properties(out IISConfigurationData iiConfigData);

    [LibraryImport(AspNetCoreModuleDll)]
+    [SuppressMessage("LibraryImportGenerator", "SYSLIB1051:Specified type is not supported by source-generated P/Invokes", Justification = "The enum is handled by the runtime.")]


What's the issue?

It doesn't appreciate that the generated type HTTP_REQUEST_PROPERTY is an enum, but it works fine.

BrennanConroy · 2023-11-03T17:40:25Z

src/Servers/IIS/IntegrationTesting.IIS/src/IISDeploymentResult.cs

@@ -43,5 +43,10 @@ public HttpClient CreateClient(HttpMessageHandler messageHandler)
        };
    }

+    public void Dispose()
+    {
+        HttpClient.Dispose();


Weren't there issues with this when Hao tried it before?

We finally got confirmation that this issue is a bug that's been fixed in a future version of IIS/Windows. I don't recall any issues with the mitigation?

src/Shared/HttpSys/RequestProcessing/NativeRequestContext.cs

Tratcher · 2023-11-03T20:31:54Z

Are there details about where the native overlapped issue shows up and what our reaction will be when the issue is fixed?

I'll file a followup issue once this is merged. When the generator is fixed we should be able to remove additional pinvoke definitions from HttpApi such as HttpWaitForDisconnectEx, HttpReceiveRequestEntityBody, etc. Maybe all of them?

danmoseley · 2023-11-03T23:39:35Z

nice to see 1500 lines of code vanish.

ghost · 2023-11-03T23:39:40Z

Hi @danmoseley. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

This reverts commit 7f18f8f. # Conflicts: # eng/Versions.props # src/Servers/HttpSys/src/AsyncAcceptContext.cs # src/Servers/HttpSys/src/AuthenticationManager.cs # src/Servers/HttpSys/src/Microsoft.AspNetCore.Server.HttpSys.csproj # src/Servers/HttpSys/src/NativeMethods.txt # src/Servers/IIS/IIS/src/Microsoft.AspNetCore.Server.IIS.csproj

Tratcher self-assigned this Sep 13, 2023

dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Sep 13, 2023

Tratcher requested review from joperezr and wtgodbe September 13, 2023 18:56

wtgodbe approved these changes Sep 13, 2023

View reviewed changes

wtgodbe reviewed Sep 14, 2023

View reviewed changes

eng/Dependencies.props Show resolved Hide resolved

danmoseley reviewed Sep 14, 2023

View reviewed changes

src/Shared/HttpSys/RequestProcessing/NativeRequestContext.cs Show resolved Hide resolved

MichaelSimons reviewed Sep 18, 2023

View reviewed changes

src/Servers/IIS/IIS/src/Microsoft.AspNetCore.Server.IIS.csproj Outdated Show resolved Hide resolved

Tratcher added 17 commits September 27, 2023 11:25

Use generated PInvokes and exchange types

b2281ab

IIS, cleanup

99b0e19

Client Certs

b0c94a1

Response, data chunks

ff71d9e

Version, direct pinvokes

0469c47

Response headers

d716113

More delete

7704525

Flags

da3c8ca

Consolidate

ebbce1f

ExcludeFromSourceBuild

9f66273

Cleanup

933163e

ordering

9d8bae5

ordering

d4d3ed9

Exclude IIS from source build

bb77b1b

Scope imports

f9ad985

Remove UnsafeNclNativeMethods

b9e4eea

Clean up shared code

09966ae

Tratcher force-pushed the tratcher/pinvoke branch from 97af338 to 09966ae Compare September 27, 2023 22:30

Tratcher requested a review from jkoritzinsky October 3, 2023 17:46

ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Oct 11, 2023

Tratcher mentioned this pull request Oct 11, 2023

Friendly overloads for NativeOverlapped not working microsoft/CsWin32#1066

Open

ghost removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Oct 20, 2023

ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Oct 28, 2023

ghost removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Oct 30, 2023

BrennanConroy approved these changes Nov 3, 2023

View reviewed changes

Tratcher and others added 2 commits November 3, 2023 13:27

Update dependency, cleanup

0cba7ef

Merge branch 'main' into tratcher/pinvoke

a3d4608

Tratcher mentioned this pull request Nov 3, 2023

SourceGen NativeOverlapped PInvokes #51863

Open

Tratcher enabled auto-merge (squash) November 3, 2023 20:42

Tratcher merged commit 7f18f8f into dotnet:main Nov 3, 2023
26 checks passed

Tratcher deleted the tratcher/pinvoke branch November 3, 2023 22:04

mgravell mentioned this pull request May 3, 2024

Experimental revert of P/Invoke layer #55519

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use generated PInvokes and exchange types #50685

Use generated PInvokes and exchange types #50685

Tratcher commented Sep 13, 2023 •

edited

Loading

wtgodbe left a comment

Tratcher commented Sep 14, 2023

joperezr commented Sep 14, 2023

danmoseley commented Sep 27, 2023

Tratcher commented Sep 27, 2023

adityamandaleeka commented Sep 28, 2023

AaronRobinsonMSFT commented Sep 29, 2023

ghost commented Oct 11, 2023

AaronRobinsonMSFT commented Oct 11, 2023

Tratcher commented Oct 11, 2023 •

edited

Loading

Tratcher commented Oct 11, 2023

Tratcher commented Oct 20, 2023

azure-pipelines bot commented Oct 20, 2023

Tratcher commented Oct 20, 2023

ghost commented Oct 28, 2023

Tratcher commented Oct 30, 2023

azure-pipelines bot commented Oct 30, 2023

BrennanConroy left a comment

BrennanConroy Nov 3, 2023

Tratcher Nov 3, 2023

BrennanConroy Nov 3, 2023

Tratcher Nov 3, 2023

BrennanConroy Nov 3, 2023

Tratcher Nov 3, 2023

BrennanConroy Nov 3, 2023

Tratcher Nov 3, 2023

Tratcher commented Nov 3, 2023

danmoseley commented Nov 3, 2023

ghost commented Nov 3, 2023

Value	Meaning
0 (zero)	Only the request headers are retrieved; the entity body is not copied.
HTTP_RECEIVE_REQUEST_FLAG_COPY_BODY	The available entity body is copied along with the request headers. The pEntityChunks member of the HTTP_REQUEST structure points to the entity body.
HTTP_RECEIVE_REQUEST_FLAG_FLUSH_BODY	All of the entity bodies are copied along with the request headers. The pEntityChunks member of the HTTP_REQUEST structure points to the entity body.

Use generated PInvokes and exchange types #50685

Use generated PInvokes and exchange types #50685

Conversation

Tratcher commented Sep 13, 2023 • edited Loading

wtgodbe left a comment

Choose a reason for hiding this comment

Tratcher commented Sep 14, 2023

joperezr commented Sep 14, 2023

danmoseley commented Sep 27, 2023

Tratcher commented Sep 27, 2023

adityamandaleeka commented Sep 28, 2023

AaronRobinsonMSFT commented Sep 29, 2023

ghost commented Oct 11, 2023

AaronRobinsonMSFT commented Oct 11, 2023

Tratcher commented Oct 11, 2023 • edited Loading

Tratcher commented Oct 11, 2023

Tratcher commented Oct 20, 2023

azure-pipelines bot commented Oct 20, 2023

Tratcher commented Oct 20, 2023

ghost commented Oct 28, 2023

Tratcher commented Oct 30, 2023

azure-pipelines bot commented Oct 30, 2023

BrennanConroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Nov 3, 2023

danmoseley commented Nov 3, 2023

ghost commented Nov 3, 2023

Tratcher commented Sep 13, 2023 •

edited

Loading

Tratcher commented Oct 11, 2023 •

edited

Loading