Task-Based Overlapped IO In .NET

Question

I've been working wrapping the Windows API's Overlapped methods in the more usable Task based approach. There isn't much information out there on doing this, so I've been flying blind apart from a few blog posts. I was hoping to get feedback on what I've come up with so far.

An example method is as follows.

private Task<byte[]> GetSectors(Int32 sectorIdx,Int32 sectorCount,Int32 sectorSize)
{
 //Create a buffer to read into.
 //The SafeGlobalBuffer class is defined elsewhere, 
 //and uses Marshal.AllocGlobal as a backend.
 //It's being used as a backend because it supports alignment, 
 //which is required for some forms of overlapped IO. This is not demonstrated.
 SafeBuffer buffer = new SafeGlobalBuffer(sectorSize * sectorCount);
 //Create a task completion source. 
 TaskCompletionSource<byte[]> tcs = new TaskCompletionSource<byte[]>();
 //Set up the overlapped structure.
 //We don't need an event, and we're not using the old-style asynchrony pattern.
 ulong offset = (ulong)sectorIdx * (ulong)sectorSize;
 Overlapped mOverlapped = new Overlapped(
 (int)(offset&0xffffffff), 
 (int)(offset>>32), 
 IntPtr.Zero, 
 null);
 //We'll be using ReadFileEx, which provides it's own callback mechanism, 
 //so we can pass in null for the callback. For this to work, 
 //the handle must NOT be already bound to the threadpool.
 NativeOverlapped* pOverlapped = mOverlapped.Pack(null, null);
 //Dispatch the Read command. 
 if (!ReadFileEx(
 _hDisk, 
 buffer, 
 (UInt32)buffer.ByteLength, 
 pOverlapped, 
 (errorCode, bytesTransferred, pOverlappedLocal) =>
 {
 try
 {
 if (errorCode != 0)
 {
 //Something terrible has happened. Let the waiter know about it.
 tcs.SetException(new Win32Exception((int)errorCode);
 }else
 {
 //Get the data out of Global memory and tell the waiter about it.
 byte[] readData = new byte[buffer.ByteLength];
 buffer.ReadArray<byte>(0, readData, 0, readData.Length);
 tcs.TrySetResult(readData);
 }
 }
 finally
 {
 //No matter what, clean up.
 Overlapped.Unpack(pOverlappedLocal);
 Overlapped.Free(pOverlappedLocal);
 buffer.Dispose();
 }
 }))
 {
 //ReadFileEx failed for some reason.
 Overlapped.Unpack(pOverlapped);
 Overlapped.Free(pOverlapped);
 tcs.SetException(new Win32Exception());
 buffer.Dispose();
 }
 //Return the task for tracking.
 return tcs.Task;
} 

ReadFileEx is defined as follows:

 [DllImport("kernel32.dll", SetLastError = true)]
 private static unsafe extern Boolean ReadFileEx(
 SafeFileHandle hDisk,
 SafeBuffer buffer, 
 UInt32 bytesToRead, 
 NativeOverlapped* lpOverlapped, 
 IOCompletionCallback callback);

Some things I've noticed and am worried specifically about:

1. Both buffer and tcs are managed objects that don't get pinned in any way. My testing seems to show they don't get GCd or moved, but I'm not sure why. This is worrying, because Overlapped.Pack provides a parameter specifically for pinning a buffer, which makes me think there's some risk of this happening.

2. I'm a bit unclear on when to use the Overlapped.Unpack method. The examples I found were all over the place with this, and didn't seem to give much in the way of explanation. It seemed the safest thing to do was to do the same steps of preparing the native overlapped in reverse.

And then there are some things I know about:

1. The integer typing is schizophrenic. .NET seems to hate working with anything that's not an Int32, which makes interop very annoying. Suggestions on this would be appriciated.

2. I could be using a byte[] instead of a SafeBuffer. This is true for this examlpe, but certain IO requires page or sector aligned memory on the host side. The only real way to do this is by using a custom buffer class or by making the calls using a raw IntPtr, neither of which is particularly appealing.

• t3chb0t: Your questions seem to be beyond code review. Besides the try/catch inside the if is something I'd never let through to a production code.
• JanDotNet: @t3chb0t: Actually, the try/catch is part of the delegate's body... But it is difficult to recognize because of the suboptimal formating.
• Benjamin: @t3chb0t If it's beyond code review, I'm not sure where it fits, because it's also not in the right format to be an SO question and it's too specific to post in Programming or something. Since the intention is to simply garner feedback, I posted it here.
• Benjamin: @JanDotNet I agree the formatting for the isn't great. This has been changed so that the call to ReadFileEx does not include the function body for the callback delegate, which does make it much easier to read. Thank you.

Answer 1

Int32 

I don't see much reason to write Int32 instead of the more common int. Likewise for Boolean and UInt32.

---

TaskCompletionSource<byte[]> tcs = new TaskCompletionSource<byte[]>();

In most styles, using var is okay if the type of the variable is clear from the right side of the assignment, which is the case here, so I would write:

var tcs = new TaskCompletionSource<byte[]>();

---

ReadFileEx(
 _hDisk, 
 buffer, 
 (UInt32)buffer.ByteLength, 
 pOverlapped, 
 (errorCode, bytesTransferred, pOverlappedLocal) =>
{
 ...
})

I think the callback delegate can be garbage collected while the read is in progress, causing memory issues.

To verify, try calling GC.Collect() right after ReadFileEx().

The solution would be to store the delegate in a local variable and ensure it's alive using GC.KeepAlive().

---

if (!ReadFileEx(
 _hDisk, 
 buffer, 
 (UInt32)buffer.ByteLength, 
 pOverlapped, 
 (errorCode, bytesTransferred, pOverlappedLocal) =>
{
 ...
}))
{
 ...
}

I find this formatting fairly confusing, because the body of the if is too far from the if keyword. Either put the delegate into a local variable, or make it into a full method (though that would make closing over local variables less convenient).

---

It should not matter that buffer and tcs are not pinned, since they are not directly accessed by native code (unlike the callback delegate above)

• Benjamin: 1. I go back and forth on weather I like using the built in names or the "class" names. Microsoft doesn't seem to know which they prefer either, but the note is taken. 2. Agreed. var would be more readable and flexible. 3. That's something I hadn't thought of. I have looked at the source for the coreCLR project, which SEEMS to ensure the delegate isn't gc'd, but I'll test to make sure. 4. Agreed, and has already been corrected. 5. That makes sense. Thank you.