Windows hotplug: Mutex for callback actions #646
Labels
No labels
3rdparty
Android
API
Author does not respond
bsd
bug
build system/CI
ci-virtual-device
CMake
Contributions Welcome
Core
dependencies
device-specific
documentation
don't_merge
duplicate
enhancement
Example
Haiku
hidraw
hotplug
info required
invalid
libusb
macOS
Misc
Not an issue
question
Release
Test
Test GUI
Windows
wontfix
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-libusb/hidapi!646
Loading…
Reference in a new issue
No description provided.
Delete branch "connection-callback-mutex"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Resolves #538
it shouldn't be done under
HIDAPI_USE_DDKwe probably need to move only the lookup functions under the
HIDAPI_USE_DDKand keep thehidapi_initializedflag andhid_hotplug_contextinitialisationalso,
hid_exit()should deinitializehid_hotplug_contextas wellDidn't notice the macro check, should be fixed now.
@DJm00n Could you have a look on the code in this PR?
update error string
rename to
hid_internal_hotplug_cleanup()extract to separate
hid_internal_hotplug_init()methodextract to separate
hid_internal_hotplug_exit()methoduse tabs instead of spaces here and in other places
@ -1090,1 +1158,3 @@/* TODO: protect the handle by the context hotplug lock *//* Ensure we are ready to actually use the mutex */hid_internal_hotplug_init();seems
ReleaseMutex()call should be afterhid_internal_cleanup_hotplugs()I'd prefer native Unicode
CreateMutexWversion.@ -1090,1 +1158,3 @@/* TODO: protect the handle by the context hotplug lock *//* Ensure we are ready to actually use the mutex */hid_internal_hotplug_init();seems
hotplug_cbvariable is not needed in this method and you can just putfree(*current);here.Okay. But, theoretically, this doesn't matter, as we don't assign it any name anyway.
@DJm00n all suggestions applied, thank you!
Just noticed that the cleanup code on exit won't really do anything if any callbacks are still armed; going to fix that in a moment.
Typo
Fix spacing
Done. Unfortunatelly, this got copy-pasted into other 2 branches as well...
revert
put string on the same line
how it can compile without
;?revert
revert
no need of newline here
@ -967,3 +1034,4 @@struct hid_device_info *device = NULL;hid_hotplug_event hotplug_event = 0;(void)notify;no need of newline here
put it in one line
put it in one line
It can not; I probably shoud not have used notepad for correcting indentation
Would it be fine to throw in a small change for the hidtest (in the hotplug testing section)?
It needs a fflush(stdout) in the callback, otherwise the output gets stuck and events don't pop up immediately (sometimes at all).
Ran some tests, had to add this in, with it everything seems to work.
UPD: added the change. Should help with testing the events with the added mutex.
the spaces are inconsistent in this function
I get that it is not directly related to your change, but do you mind replacing spaces with tabs in the entier function to make it consistent?
thanks
what is
1? can we use a named constant instead?INVALID_HANDLE_VALUEperhaps?still not handled
it is more about consistent API usage
we use
*Win all other cases, better not to miss-match different API(and even if not - that is better be fixed at some point)
It is a valid starting value; it can be set to any arbitrary positive number.
Additionally, this line was only moved, it wasn't changed.
agree
looks like you're using some kind of IDE wich does its own formatting - that better be disabled, as it introduces too much code-style changes and makes the code inconsistent
unfortunately we don't have a clang-format spec for this repo (yet) so we have to keep the consistance/readability manually
Apart from the formatting issues: is there're a specific reason to init/deinit the hotplug routines during hid_init/hid_exit?
can we initialize the whoe thing only when
hid_hotplug_register_callbackis called?that way there would be less overhead for those users (all up until now) who don't use the hotplug API yet
OK, but only because of that.
The main reason here is allowing the register/deregister functions being called from multiple threads simultaneously. This is the original reason to add the mutex at all, and, unfortunately, the mutex has to be initialized somewhere before use. hid_init seemed like the most reasonable place to do so, as the chance of getting a second call to it from a different thread at the same time (before the mutex is initialized) is negligible.
If we happen to get two simultaneous register calls from two different threads with an uninitialized mutex, both could attempt to make a new one, which would lead to either thread locking a different mutex, from where we can encounter all sorts of weird behaviors, including possible memory leak scenarios where the first cached device list is leaked, or one of the callbacks is lost.
I believe the mutex initialization overhead to be a sacrifice small enough to ignore.
UPD: I found a way to avoid the mutex initialization with critical sectiona, investigating.
If a named constant is to be used, it would be more like
HOTPLUG_FIRST_CALLBACK_HANDLE, how does this name sound?Will do
I have to apoligize for not asking this earlier: why do we even need to allow calling register/deregister functions from different threads?
HIDAPI was never intended to have a thread-safe API in general case.
hid_init/hid_exit<->hid_enumerate<->hid_open/hid_open_path/hid_close are not thread-safe in general case (as per my testing on 4 platforms) and I don't see good reason why making register/deregister functions as thread safe
Originally I assumed that mutex is needed for some kind of data syncronisation internally and I didn't actually payed attention to what actually was being done.
That too, by the way, the events are processed in a background thread, and the register calls can potentially interfere with the processing.
If we don't want to ensure the register/deregister call safety, I could move the initialization to the register function. However, I don't know how good of an idea it would be to deinitialize it in the deregister function, need to think on that for a bit.
go for it
I see two trade offs here:
NOTE: in any case, user may not call deregister function at all, and that could be fine, i.e.
hid_closeshould gracefully perform the deinitialisation if not performed explicitly.More than that - multiple calls to deregister has to be safe (for the similar reason why
free(NULL)is safe).Took the asymmetric approach. The register call now initializes the synchronization. It is only deleted in hid_exit. We lose nothing by keeping it initialized.
Replaced the mutex with a CriticalSection. It is the same in function (for unnamed mutexes, at least), but it works ~20 times faster.
@ -78,6 +79,10 @@ typedef LONG NTSTATUS;#define MAX_STRING_WCHARS_USB 126/* The value of the first callback handle to be given upon registration */now it is perfectly clear, thanks
even if we've failed here - we probably still need to clear the value of
notify_handleso we do not attempt to Unregister it once moreLooks very good.
The only thing needs fixing (apart of one of my last comments) is the formatting. I'm ready to fix it myself at some point.
Done. That should do it.
Simple test results seem to be good. Tested under Windows.
Unplug and plug of Plantronics Headset (USB composite device)
Unplug and plug of Xiaomi USB mouse receiver
Unplug and plug of USBasp programmer (USB Composite device)
test using hidtest
Testing again under Windows 10 Dell Laptop to make sure it works -- the results are good.
Another test with WIndows 11 Acer laptop and it is also good.
I am looking forward of this getting merged :)
@ -183,19 +188,44 @@ err:#endif /* HIDAPI_USE_DDK */I think this should be a BOOL type.
Here you are using the
callbackwhich potentially freed already I would rather save its next value beforehand,also shouldn't the
hid_internal_hotplug_cleanupcalled here?This would free the next one instead of the current because it was already set up for the next one.
This is actually done intentionally so that the hotplug_cbs pointer doesn't get the chance to be changed to NULL by any background thread (can happen even through an internal thread if the last callback returns a value that deregisters it)
@ -183,19 +188,44 @@ err:#endif /* HIDAPI_USE_DDK */Given that it's declared as
typedef int BOOL;I don't see much difference :)Actually, no, if
freeis called then the execution never reaches this point, ascontinuewill jump it to the beginning of the cycle, andcallbackwill be reset.@ -367,9 +397,18 @@ HID_API_EXPORT const char* HID_API_CALL hid_version_str(void)return HID_API_VERSION_STR;@ -183,19 +188,44 @@ err:#endif /* HIDAPI_USE_DDK */The difference in discipline and code consistency. It does matter a lot.
and
BOOLis way more explicit/readable@ -930,40 +1029,9 @@ void HID_API_EXPORT HID_API_CALL hid_free_enumeration(struct hid_device_info *d}@ -183,19 +188,44 @@ err:#endif /* HIDAPI_USE_DDK */it is set to next one in the next line (at least as of this moment)
@ -967,3 +1034,4 @@struct hid_device_info *device = NULL;hid_hotplug_event hotplug_event = 0;(void)notify;looks like some ("default"?) IDE settings
@ -1016,10 +1085,10 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context,}@ -379,9 +418,66 @@ int HID_API_EXPORT hid_init(void)hidapi_initialized = TRUE;@ -379,9 +418,66 @@ int HID_API_EXPORT hid_init(void)hidapi_initialized = TRUE;Only thanks to @xQwexx , the error was found and fixed on the same day