search

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

Review and Improve Overlapped IO Policies #419

Closed BenjaminHolland closed 4 years ago

BenjaminHolland commented 5 years ago

Doing overlapped IO with the transition between managed and unmanaged code is significantly difficult. While the overlapped structure is recreated, and several methods have been provided, without additional work the process for actually using them is unclear and extremely error prone.

The .NET framework and .NET core both contain facilities that make this process easier. Examples are Overlapped/NativeOverlapped and ThreadPoolBoundHandle. Since NativeOverlapped shares the same layout as the provided OVERLAPPED struct, using a pointer cast works, but seems to me a bit messy.

I'd like to start a discussion about how this library should handle this particular area.

BenjaminHolland commented 5 years ago

Some useful examples: https://codereview.stackexchange.com/questions/135646/task-based-overlapped-io-in-net https://gist.github.com/BenjaminHolland/96b6e97856ffd0df532c5ed6def5de9d

AArnott commented 5 years ago

I have no plans to use the overlapped I/O at present, nor any experience to draw on. I'm very interested in your take on it since you're giving it thought. Removing our own OVERLAPPED struct in favor of the one in .NET makes a lot of sense to me given it is expressly documented as compatible with the one in the OS. If you want to send a PR with that change, I'd welcome that.

We might run into an issue with PCL target compatibility. Let me know if that's the case.

BenjaminHolland commented 5 years ago

@AArnott I'd certainly be willing to do a PR, but I'm not at all familiar with targeting multiple platforms. Is there a quick rundown you can give me on that subject, or a resource you can point me to?

The biggest problem with using P/Invoked overlapped IO is that since everything is async the GC has a chance to move or delete stuff that's actually being used by the completion routine. The way the standard libraries have solved this is by rolling that concern into the Overlapped class, which manages stack stuff* and automatically manages the buffer you're using. It also allows you to associate a callback with every operation, which is functionality that seems to have been provided in the Win32 API in the ReadFileEx and WriteFileEx and other such methods.

Newer .NET Core version seem to have expanded this support since I last worked with this, providing PreallocatedOverlapped, which seems to be an efficient overlapped factory, and ThreadPoolBoundHandle, which seems to manage the association between the handle and the OS thread pool, which seems to be required for the overlapped operation to actually complete. This ties in with my other issue (which I can't find at the moment) about the type of SafeHandle-derived method arguments.

Having your own overlapped struct is fine, but makes working with methods that use overlapped io require either manual GC management, or boilerplate pointer casts.

* I think. It's been a couple years since I looked at the source, and I wasn't super clear on what it was doing at the time...

AArnott commented 5 years ago

The only thing you should have to worry about as far as multi-targeting in this repo is that if you're defining any p/invoke methods into Store-banned APIs that those methods be defined in the right file (the one with the rest of the store-banned APIs). That ensures that the PInvoke libraries can be used from all kinds of apps, without causing store apps to be rejected due to calling banned APIs.