hid_close will close immediately on win32

[LianYangCn]

see signal11/hidapi#416

In multi-thread environment, if the hid_read thread is different from the hid_close thread, the hid_read function will block the thread exiting, at the wait object operation.

This fix just set the event, after cancel io, so that, hid_read not wait anymore and exiting read

[Youw]

maybe this fixes one issue, but there still others, that you'll have in the same scenario, and this fix is not sufficient:

what you're trying to fix:
thread1: waits on: hid_read_timeout(dev, data, size, some_positive_timeout);
thread2: call hid_close(dev);
thread1 hangs on WaitForSingleObject(...)

what you expect after your fix:
thread1: waits on: hid_read_timeout(dev, data, size, some_positive_timeout);
thread2: call hid_close(dev);
thread1 exits immediately here:

hidapi/windows/hid.c

Line 712 in e739dc1

return 0;

what can actually happen:
thread1: waits on: hid_read_timeout(dev, data, size, some_positive_timeout);
the data is arrived from the device and WaitForSingleObject returns success -> at this point OS switches running context to thread2
thread2: call hid_close(dev); -> device is closed/destroyed -> context switched to thread2:
thread2 crashed, e.g. here:

hidapi/windows/hid.c

Line 719 in e739dc1

res = GetOverlappedResult(dev->device_handle, &dev->ol, &bytes_read, TRUE/*wait*/);

---

HIDAPI Windows implementation does not support calling hid_read/hid_write and hid_close from multiple threads mainly because it wasn't designed in the right way at the first place

if you need to use hid_read in a different thread (I'd say it is a very natural need), and want it to work correctly, I suggest (manually in your project):

1. make sure you don't access the device in any other place/thread, either by mutex/guard, or by closing your reading thread completely (e.g., if you have a dedicated single thread for hid_read for this specific device)
2. only then hid_close the device

[Youw]

will update README eventually: #45

[LianYangCn]

@Youw
Hi,

I think this is not a feature, but more like a bug.

hid_close will call CancelIo

hidapi/windows/hid.c

Line 825 in e739dc1

CancelIo(dev->device_handle);

and,

CanncelIo: Cancels all pending input and output (I/O) operations that are issued by the calling thread for the specified file. The function does not cancel I/O operations that other threads issue for a file handle.

And if a thread already waiting a hEvent in OVERLAPPED structure, and this will cause

hidapi/windows/hid.c

Line 708 in e739dc1

res = WaitForSingleObject(ev, milliseconds);

hang forever.

Calling CancelSynchronousIo is not a bad idea even with synchronous. This PR make sure resource are free.
So, this PR fix this issue.

BTW,

what you expect after your fix:
thread1: waits on: hid_read_timeout(dev, data, size, some_positive_timeout);
thread2: call hid_close(dev);
thread1 exits immediately here:
https://github.com/libusb/hidapi/blob/e739dc117c575cfc8246a7056a0c321e81142ab0/windows/hid.c#L712

This IS what actually happen.

[Youw]

I think this is not a feature, but more like a bug.

please do not mistaken a bug with a documented (not yet) API limitation.

Now, the quote:

CanncelIo: Cancels all pending input and output (I/O) operations
that are issued by the calling thread for the specified file. The function does not cancel I/O operations that other threads issue for a file handle.

If you pay enough attention, the read operation is issued by a thread1, where hid_read happens, and you're trying to call CancelIo from thread2, where hid_close happens.
Documentation explicitly states, that you shouldn't call it that way. Even though it is working, it is not guaranteed to.

This IS what actually happen.

I'm sure it is what actually happens most of the time.
In multi-threaded environment you cannot think of one path of execution as the only possible one.
You have to think of all possible paths of executions, and be sure, that one (more than one) of them - is a crash.

So, this PR fix this issue.

It hides the issue deep in the implementation details and when someone will hit it (with 0.01% chance) - it will take no fun debugging it.

[LianYangCn]

Sorry. I mean this is not a thread related issue, it is about resource release or the state issue.

the hEvent may reset by:

hidapi/windows/hid.c

Line 692 in e739dc1

ResetEvent(ev);

and on closing, CancelIo does not set it back, hid_close does not set it back neither.

Aside from the influence of the thread, setting this hEvent back to the original state, I think it is more reasonable.

[LianYangCn]

will update README eventually: #45

HIDAPI Windows implementation does not support calling hid_read/hid_write and hid_close from > > > multiple threads mainly because it wasn't designed in the right way at the first place

I agree those.

[Youw]

and on closing, CancelIo does not set it back, hid_close does not set it back neither

Set it back to what?

---

ResetEvent documentation:

Sets the specified event object to the nonsignaled state.

Default object state on creation is nonsignaled:

hidapi/windows/hid.c

Line 158 in e739dc1

dev->ol.hEvent = CreateEvent(NULL, FALSE, FALSE /*initial state f=nonsignaled*/, NULL);

---

NOTE: an Event object may be Closed in any of its possible states (signaled or nonsignaled) safely, unless you found a documentation, that states the opposite.

---

Thank you for your PR, but there is no resource leak or a state issue.