Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

win32: Hangs forever in hid_read_timeout() when calling hid_close() #649

Closed
dontech opened this issue Nov 28, 2023 · 6 comments
Closed

win32: Hangs forever in hid_read_timeout() when calling hid_close() #649

dontech opened this issue Nov 28, 2023 · 6 comments
Labels
Windows Related to Windows backend

Comments

@dontech
Copy link

dontech commented Nov 28, 2023

As the title states, the problems reported earlier around GetOverlappedResult() is still present.

There where attempted fixed in:

#128
signal11/hidapi#88

But for some simple scenarios this is still the case:

  1. Read using hid_read_timeout() in a background thread.
  2. Attempt to close using the main thread using hid_close()
  3. Hang forever in GetOverlappedResult().
@Youw
Copy link
Member

Youw commented Nov 28, 2023

It has nothing to do with #128 specifically.
HIDAPI is not thread safe. You should not attempt to hid_close while perforing other hid_* funcitons on the same device. That is undefined behavior.
Some info about it: #45

@Youw Youw closed this as not planned Won't fix, can't repro, duplicate, stale Nov 28, 2023
@Youw
Copy link
Member

Youw commented Nov 28, 2023

Oh, and BTW: 4d02971 is not really a fix. There is still an undefined behavior and you'll be getting crashes e.g. here because hid_read* would attempt to access the device that is already closed/freed.

@dontech
Copy link
Author

dontech commented Nov 29, 2023

Oh, and BTW: 4d02971 is not really a fix. There is still an undefined behavior and you'll be getting crashes e.g. here because hid_read* would attempt to access the device that is already closed/freed.

That is only true, due to the library missing synchronization function calls during read()/write() operations and close().
I do not see any obvious reason why hidapi, in difference to other similar I/O libraries should not be thread-safe using simple O/S provided mechanisms. But i see that you are right that this is not currently the case.

I also mentioned this in: #133

e.i, by implementing a simple EnterCriticalSection()/ExitCriticalSection() construct in the win32 port, the problem you are describing would not be there. If I find the time, I will propose a patch to show how this would look.

@Youw
Copy link
Member

Youw commented Nov 29, 2023

You just faild to consider all multi-threading scenarios.
The failure case: #651 (comment)

@dontech
Copy link
Author

dontech commented Nov 29, 2023

You just faild to consider all multi-threading scenarios.

What you are stating is true and is basic teachings in operating system problematics.
An aspect of this is briefly discussed in the man page for close on Linux:

https://man7.org/linux/man-pages/man2/close.2.html

This is discussed probably in all O/S classes at least once....

However, I have never seen this as an argument to not have any synchronization, and to enforce completely random behavior as a result. Argument is correct, conclusion is odd.

With this logic, we should remove all synchronization from all libraries, drivers, operating system, etc. on the planet, as they, like here, suffer the same problem.

The rest of the world solves this with external synchronization and a note in the manual that states that "any call to any file operation after close() is not defined and not supported".

I do not see why a simple library like hidapi should be any different.

You are limiting hidapi to never be used in a multi-threaded environment because of an irritation with the unsolvable close() problem that haunts every file system programmer. Live with it, and document that hidapi does not not, like other systems, solve this.

@Youw
Copy link
Member

Youw commented Nov 29, 2023

Lets continue this discussin in one place: #133

@libusb libusb locked and limited conversation to collaborators Nov 29, 2023
@mcuee mcuee added the Windows Related to Windows backend label Jan 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Windows Related to Windows backend
Projects
None yet
Development

No branches or pull requests

3 participants