Windows hotplug: Mutex for callback actions #646
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Youw
reviewed
Nov 21, 2023
k1-801
force-pushed
the
connection-callback-mutex
branch
from
87a8bc6
to
04d2eba
Compare
|
@DJm00n Could you have a look on the code in this PR? |
DJm00n
suggested changes
Nov 23, 2023
DJm00n
reviewed
Nov 23, 2023
DJm00n
reviewed
Nov 23, 2023
|
@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. |
DJm00n
reviewed
Nov 26, 2023
DJm00n
reviewed
Nov 26, 2023
DJm00n
reviewed
Nov 28, 2023
DJm00n
suggested changes
Nov 28, 2023
windows/hid.c
Outdated
| @@ -943,24 +1010,23 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, | |||
| return ERROR_SUCCESS; | |||
| } | |||
|
|
|||
| device = hid_internal_get_device_info(event_data->u.DeviceInterface.SymbolicLink, read_handle); | |||
| device = hid_internal_get_device_info(event_data->u.DeviceInterface.SymbolicLink, | |||
DJm00n
suggested changes
Nov 28, 2023
DJm00n
suggested changes
Nov 28, 2023
|
Would it be fine to throw in a small change for the hidtest (in the hotplug testing section)? 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. |
DJm00n
approved these changes
Nov 30, 2023
Youw
reviewed
Nov 30, 2023
Youw
reviewed
Nov 30, 2023
Youw
reviewed
Nov 30, 2023
Youw
reviewed
Nov 30, 2023
Youw
reviewed
Nov 30, 2023
There was a problem hiding this comment.
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_callback is called?
that way there would be less overhead for those users (all up until now) who don't use the hotplug API yet
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. |
|
I have to apoligize for not asking this earlier: why do we even need to allow calling register/deregister functions from different threads? 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. More than that - multiple calls to deregister has to be safe (for the similar reason why |
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. |
Youw
reviewed
Dec 1, 2023
Youw
reviewed
Dec 1, 2023
Youw
reviewed
Dec 1, 2023
k1-801
changed the title
|
Simple test results seem to be good. Tested under Windows.
test using hidtest |
k1-801
force-pushed
the
connection-callback-mutex
branch
from
9a098b2
to
a015e6e
Compare
|
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.
|
k1-801
changed the title
xQwexx
reviewed
Mar 27, 2024
Youw
reviewed
Apr 6, 2024
Youw
reviewed
Apr 6, 2024
Youw
reviewed
Apr 6, 2024
Youw
reviewed
Apr 6, 2024
Youw
reviewed
Apr 6, 2024
Youw
reviewed
Apr 6, 2024
Youw
approved these changes
Apr 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #538