HttpReceiveHttpRequest OVERLAPPED* parameter should be marked as a long-pin pointer

[AArnott]

In microsoft/CsWin32#1066, @Tratcher points out that CsWin32's generated code for the HttpReceiveHttpRequest function causes AVs when using the friendly overload. I believe the problem is that we pin an OVERLAPPED structure and pass in a pointer that will only be valid during the lifetime of the method call. For this method and parameter in particular, I guess the pointer is expected to be valid for the duration of an async I/O operation, which means C# must not accept a struct on the stack in the friendly overload.

But this is a deviation from the norm for Win32, where most pointers are not held by the OS beyond the scope of the method call.
Can win32metadata attribute this method parameter somehow so that CsWin32 (and other projections) can realize that the application level code must own the lifetime of the pointer instead of the marshaling layer?

[riverar]

I wouldn't classify this as a deviation; established asynchronous I/O procedure would have you keep OVERLAPPED around until asynchronous I/O operations complete.

Since HttpReceiveHttpRequest can be used synchronously or asynchronously, it's not clear how/which markings will help here. Perhaps you can just use the OVERLAPPED structure as a hint during codegen in the short term?

[riverar]

Just expanding on this a bit.

Sounds like the ask is to add an async-specific flag as such:

[DllImport("HTTPAPI.dll", ExactSpelling = true, PreserveSig = false)]
public unsafe static extern uint HttpReceiveHttpRequest(
  [In] HANDLE RequestQueueHandle,
  [In] ulong RequestId,
  [In] HTTP_RECEIVE_HTTP_REQUEST_FLAGS Flags,
+ [TheoreticalAsyncFlags(MustRemainValidUntilComplete)][Out][MemorySize(BytesParamIndex = 4)] HTTP_REQUEST_V2* RequestBuffer,
  [In] uint RequestBufferLength,
+ [TheoreticalAsyncFlags(MustRemainValidUntilComplete)][Optional][Out] uint* BytesReturned,
+ [TheoreticalAsyncFlags(MustRemainValidUntilComplete)][Optional][In] OVERLAPPED* Overlapped
);

It may make more sense to detect OVERLAPPED use and assume all pointers must remain valid until the async operation completes.

[AArnott]

Yes, we certainly could special case OVERLAPPED. But if there are other APIs, slowly building up a list of such special APIs in each projection would be more buggy and less efficient that maintaining that list directly in the metadata by attributing them.

Maybe my 'deviation' adjective was the wrong word. Whatever win32 does is normal for win32 by definition. But it's uncommon, as every other function I've seen doesn't hold onto the pointer that is passed to it. But I suspect my exposure is limited, and there may be others.

[riverar]

If we fix up the overlapped parameters to include Out markings, we're good right? You should just pass that through right, leaving the user to manage the lifetime of that pointer?

[AArnott]

No, this doesn't have anything to do with the data direction annotation, AFAIK. Although over at cswin32, the user claims that win32 is in fact mutating the [in] data, and if that's true, we should certainly correct the headers and metadata.

When the metadata takes a pointer to a struct, we expose it as a pointer to C#, and also generate an overload that takes just the struct, and we pin it for the lifetime of the call. So we need a special marker on this API to indicate that the API will hold the pointer longer than the function call so that cswin32 won't assume it knows the length of time that the pointer pin must live, and therefore it will only emit pointer functions.

[riverar]

Ah, got it, thanks. Any name suggestions? We probably don't need to get crazy here like my previous example ([TheoreticalAsyncFlags(MustRemainValidUntilComplete)]). Maybe a simpler [Async] to sit next to [In][Out]?

[AArnott]

How about [PointerRetained]?

The fact that the function happens to be implementing async functionality is irrelevant, IMO. The crux of it is the function keeps the pointer for later use.

[riverar]

That's fair. I like that. Any feelings on shortening to [Retained]? Feels a little duplicitous when viewed next to pointer parameters.

[AArnott]

Sure. [Retained] sounds good.

And to whoever fixes this bug in the metadata, per microsoft/CsWin32#1066 (comment), this API should be marked as in/out, not just in. The header files themselves should probably be fixed too.

[riverar]

I can take care of both, since you/I have the most context here.