A number of issues with the overlapped I/O code: do we need it?

[jnm2]

What was the purpose of having HidDevice.ReadData and WriteData use overlapped I/O, since they block anyway and aren't asynchronous? My understanding is that the overhead of overlapped I/O is worth the throughput only when the thread is freed to do other work instead of blocking.

ReadData: https://github.com/mikeobrien/HidLibrary/blob/master/src/HidLibrary/HidDevice.cs#L619-L672
WriteData: https://github.com/mikeobrien/HidLibrary/blob/master/src/HidLibrary/HidDevice.cs#L560-L594

Issues:

1. CreateEvent is being called with bManualReset: false even though the docs say over and over that the event must be a manual-reset event and that there are problems with auto-reset events: https://docs.microsoft.com/en-us/windows/win32/sync/synchronization-and-overlapped-input-and-output

2. CreateEvent is being called with bInitialState: true, so no waiting has ever been happening. This is easy to demonstrate by calling WaitForSingleObject without calling ReadFile and seeing that it returns WAIT_OBJECT_0. This means that even if reads haven't completed, the buffer is read and returned to the caller instantly! (Maybe Data just contains 0's #112 is related?)

3. When I fix the previous two points, overlapped reads never return. I'm not sure what's wrong with the code yet.

4. WaitForSingleObject is being called rather than using the managed WaitHandle.WaitOne method. The problem with this is that the .NET runtime is out of the picture for the entire duration. During managed blocking waits, the runtime is online. I'm fuzzy on what this actually means other than that Thread.Abort won't work during a managed blocking call. I think there might have been implications for garbage collection as well.
   Also, I think all the native code can be replaced with the managed ManualResetEvent.

5. CancelIoEx is being called on the event handle, but it only makes sense to call this method on file handles. I'm guessing that this means the event handle is leaked. CloseHandle should be what is called.

Before putting effort and investigation into it, I wanted to know for sure that there is still an ongoing goal that requires keeping the overlapped I/O.

/cc @amullins83 @mikeobrien

[amullins83]

Thanks for taking the time to look into this. I became aware of this project while working on custom software for device my company was making based on a TI chip that presented itself as an HID device.

I ran into problems trying to implement our software with this library, so I eventually figured out how to use the C-based HIDAPI project by Signal11, which was what TI's sample application used. I'm not convinced the issues I had were caused by this library's implementation, because they eventually identified a problem with the device's output code.

I present my story because I feel like you most likely have discovered a root issue that is worth fixing. It's my opinion that we should try to implement these changes as major version update on a feature branch and encourage others to test the new version with all their devices before we merge to master.

I no longer work at the company where I was working on a custom device, but I think I have a Griffin Powermate lying around somewhere to test with. We'll want to at least get people using the other devices for which we have test projects (Honeywell 4000, Logitech Gamepad, and MagTech card reader).

[jnm2]

Maybe this gets at the crux of my question: if we secretly ignored the DeviceMode.Overlapped setting and always used non-overlapped I/O, what would the difference be for the users of this library? (I'm not saying we should do this, but the answer would give an understanding of the goal that we should be maintaining whatever we do.)

[amullins83]

I'm not sure. We just need to test everything and track what changes.

[jnm2]

Oh! I noticed that the timeout parameter is silently ignored if you don't specify DeviceMode.Overlapped. Maybe the only reason for overlapped I/O was to enable timeouts, and maybe not. How safe would you say it would be to:

• Stop silently ignoring the timeout with DeviceMode.NonOverlapped by using the SetCommTimeouts API
• Do everything with non-overlapped I/O now that it has reached parity with overlapped I/O (we think)
• Obsolete the DeviceMode enum? I don't think this is a device mode, just a Windows API convenience. Can the actual device actually tell the difference at all? I'm not knowledgable here.

Or should we preserve (and learn how to fix) the use of overlapped I/O just in case there is some difference that we aren't thinking of?

[jnm2]

Hans Passant is pretty knowledgeable. On a question involving reading from a serial port (https://stackoverflow.com/a/18220867) he says:

In other words, you have not yet found a good reason to use overlapped I/O. You will get exactly the same outcome by using a synchronous ReadFile() call. It will block, just like your current code does, until the serial port has data available.

So just don't bother with it.

That's not a guarantee that we aren't missing anything, but it does increase my confidence.

[jnm2]

Chatting about this a bit at https://discordapp.com/channels/143867839282020352/312132327348240384/760576302121615442 in https://aka.ms/csharp-discord, it's sounding like SetCommTimeouts would not work for concurrent per-thread, per-operation timeouts with different timeouts. One thread's SetCommTimeouts call could happen between the SetCommTimeouts call and the ReadFile call of another thread for the same handle. So I'm thinking that the use of overlapped I/O must stay.

[amullins83]

Impressive research! I haven't heard about SetCommTimeouts before. Is there a way to set a global timeout once for the application before any I/O starts? That way you could avoid any threading weirdness. It seems like just some basic Windows APIs would allow us to set up a kind of "dead man switch" before initiating a blocking I/O call, where the application would automatically ignore the result of the I/O operation unless it completes within a certain time.

That would obviously require some design work, and since Mike and I have both moved onto other things, it might be up to you to figure out which way this library should go.

[jnm2]

SetCommTimeouts is great for when you have a single timeout setting per file handle. But the HidDevice.ReadData method takes an int timeout parameter, which means that timeouts can be different between two concurrent calls to ReadData for the same file handle or concurrent calls to ReadData and WriteData. I don't think we can support the int timeout parameter without losing the timeout setting due to race conditions when SetCommTimeouts is called from two different threads concurrently for the same file handle.

Any locking we do to avoid the race condition would prevent ReadData and WriteData from being used concurrently on different threads, because the exclusive section would have to start before SetCommTimeouts and end after the ReadFile call. So really I think the only way to stop using overlapped I/O and still have timeouts is to document that HidDevices and the handles they expose are not thread safe (must only be used from one thread at a time).

[jnm2]

Another goal we might have is to replace the code with FileStream (the proof of concept seemed to work) which would also open up the possibility of true (no-thread) async:

private static FileStream OpenReadHidDevice(string devicePath)
{
    var handle = Kernel32.CreateFile(
        devicePath,
        Kernel32.ACCESS_MASK.GENERIC_READ,
        Kernel32.FILE_SHARE.READ | Kernel32.FILE_SHARE.WRITE,
        lpSecurityAttributes: IntPtr.Zero,
        Kernel32.CreationDisposition.OPEN_EXISTING,
        Kernel32.FILE_FLAG.OVERLAPPED,
        hTemplateFile: IntPtr.Zero);

    if (handle.IsInvalid) throw new Win32Exception();

    // Buffer size doesn't matter because the stream is only used for reading
    return new FileStream(handle, FileAccess.Read, bufferSize: 4096, isAsync: true);
}

Then using the stream, per-read timeouts can be controlled the idiomatic .NET way via CancellationToken:

// (The stream and buffer could be cached between reads)
using var stream = OpenReadHidDevice(device.DevicePath);
var buffer = new byte[device.Capabilities.InputReportByteLength];

var bytesRead = await stream.ReadAsync(buffer, 0, buffer.Length, cancellationToken).ConfigureAwait(false);

// https://github.com/microsoft/Windows-driver-samples/blob/master/hid/hclient/report.c#L48
RuntimeAssert.That(bytesRead == buffer.Length, "ReadFile never returns a partial HID report");

var report = new HidReport(reportSize: buffer.Length, new HidDeviceData(buffer, HidDeviceData.ReadStatus.Success));

Definition of CreateFile (click to expand)

using System;
using System.Runtime.InteropServices;
using Microsoft.Win32.SafeHandles;
// ReSharper disable InconsistentNaming
// ReSharper disable UnusedMember.Global

public static class Kernel32
{
    /// <summary>
    /// <see href="https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew"/>
    /// </summary>
    [DllImport("kernel32.dll", SetLastError = true)]
    public static extern SafeFileHandle CreateFile(string lpFileName, ACCESS_MASK dwDesiredAccess, FILE_SHARE dwShareMode, IntPtr lpSecurityAttributes, CreationDisposition dwCreationDisposition, FILE_FLAG dwFlagsAndAttributes, IntPtr hTemplateFile);

    /// <summary>
    /// <see href="https://docs.microsoft.com/en-us/windows/win32/secauthz/access-mask"/>
    /// </summary>
    [Flags]
    public enum ACCESS_MASK : uint
    {
        GENERIC_WRITE = 1u << 30,
        GENERIC_READ = 1u << 31,
    }

    /// <summary>
    /// <see href="https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew#parameters"/>
    /// </summary>
    [Flags]
    public enum FILE_SHARE : uint
    {
        READ = 1 << 0,
        WRITE = 1 << 1,
        DELETE = 1 << 2,
    }

    /// <summary>
    /// <see href="https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew#parameters"/>
    /// </summary>
    public enum CreationDisposition : uint
    {
        CREATE_NEW = 1,
        CREATE_ALWAYS = 2,
        OPEN_EXISTING = 3,
        OPEN_ALWAYS = 4,
        TRUNCATE_EXISTING = 5,
    }

    /// <summary>
    /// <see href="https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew#parameters"/>
    /// </summary>
    [Flags]
    public enum FILE_FLAG : uint
    {
        OVERLAPPED = 1 << 30,
    }
}

[jnm2]

Today I discovered https://docs.microsoft.com/en-us/uwp/api/windows.devices.humaninterfacedevice.hiddevice?view=winrt-19041.

(All UWP APIs are available in .NET Framework and .NET Core projects by adding <PackageReference Include="Microsoft.Windows.SDK.Contracts" Version="10.0.19041.1" /> or similar in the csproj.)