dotnet / pinvoke

A library containing all P/Invoke code so you don't have to import it every time. Maintained and updated to support the latest Windows OS.
MIT License
2.12k stars 222 forks source link

Add DeviceIoControlAsync helper method #504

Closed qmfrederik closed 4 years ago

qmfrederik commented 4 years ago

This is a draft PR, to make the suggestion of #500 a bit more tangible. In this case, it adds an async wrapper around DeviceIOControl like this:

Task<int> DeviceIoControlAsync(
            SafeObjectHandle hDevice,
            int dwIoControlCode,
            Memory<byte> inBuffer,
            Memory<byte> outBuffer,
            CancellationToken cancellationToken)

The bulk of the 'glue' to convert a kernel method which returns Win32ErrorCode.ERROR_IO_PENDING to an async method is in the DeviceIOControlOverlapped class. It ultimately registers a callback, which operates on a TaskCompletionSource.

In its current shape, the method task Memory<byte> rather than void*. The reason is C# doesn't let you mix unsafe and async in the same method; so if you want to call this method in an async manner, have to avoid void*. This does mean targeting netstandard2.1.

AArnott commented 4 years ago

This does mean targeting netstandard2.1.

It doesn't, actually. I pushed a commit to demonstrate.

qmfrederik commented 4 years ago

Thanks, @AArnott . I rebased the PR, changed the DeviceIoControlAsync to return a ValueTask<uint>, added a unit tests for the code path where the DeviceIoControl completes synchronously and DeviceIoControl errors out, updated the XML code comments and used a temporary file.

I looked at replacing Overlapped (the class) by NativeOverlapped (the struct). I assumed it'd mainly be a matter of setting Overlapped->hEvent so the kernel can invoke .NET code when the operation completes.

The implementation of Overlapped and NativeOverlapped look very specialized. I'm not sure whether that's because Overlapped tries to be generic and we can specialize; or because of perf concerns.

Net, if you want the further optimize this, I'll need some hints. Since it's an implementation detail (no changes to the public API), we could perhaps do that on a future PR.

AArnott commented 4 years ago

The use of the .NET Overlapped class is prohibiting the definition of your async helper method for store apps. Targeting uap10.1 would fix this, or removing use of that class. But as you say, there's no breaking API impact to fixing this later, so we can complete this as-is. Thanks!