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

Should more DeviceHandle methods take a shared self reference? #148

Closed
mxk opened this issue Oct 18, 2022 · 4 comments · Fixed by #186
Closed

Should more DeviceHandle methods take a shared self reference? #148

mxk opened this issue Oct 18, 2022 · 4 comments · Fixed by #186
Assignees

Comments

@mxk
Copy link

mxk commented Oct 18, 2022

libusb is generally thread-safe, and since DeviceHandle isn't reference-counted, it seems like more methods (e.g. reset, clear_halt, set_alternate_setting, etc.) should take &self rather than &mut self to enable shared usage via Arc<DeviceHandle<T>>. Thoughts?

@a1ien
Copy link
Owner

a1ien commented Oct 18, 2022

You can share DeviceHandle with Arc even some of function take &mut self. I think it's good protection to call this function if you own DeviceHandle. No one prevent you share DeviceHandle with Arc and call write_bulk/read_bulk from multiply thread. But what happened if you read or write in one thread and try to reset or clear_halt in other. And that's why these functions take &mut self.
We can discuss how this should be handled.

@mxk
Copy link
Author

mxk commented Oct 18, 2022

I see your point, but these decisions were already made by libusb authors, so I would say that a wrapper shouldn't place additional restrictions. Rather, it should do whatever libusb would do if reset is called in parallel with other operations, assuming that such use is valid.

In my case, I wrote a wrapper around libusb's async transfer API, which required putting the DeviceHandle behind an Arc, but then needed access to some of the &mut self methods elsewhere. Worked around it with unsafe, but didn't see a good reason why that should be needed. I can also envision use cases where you actually do want to reset the device while other things are happening to implement watchdog-like functionality.

@a1ien
Copy link
Owner

a1ien commented Oct 18, 2022

Ok. I am agree. I will try to make new update for library. But I think it will be at the end of October or begin November. It will be breaking changes.

@a1ien a1ien self-assigned this Oct 18, 2022
@mcuee
Copy link

mcuee commented Apr 30, 2023

Just take note libusb_reset() behavior is platform dependant. Under Windows it may be even driver dependant. libusb gives the freedom to the user and user can shoot themselves by calling libusb_reet() in a multithread application.

As for rusb, I think it does not really need to follow libusb in this aspect.

surban added a commit to surban/rusb that referenced this issue Nov 13, 2023
Change the following methods to accept a non-exclusive reference:
- DeviceHandle::set_active_configuration
- DeviceHandle::unconfigure
- DeviceHandle::reset
- DeviceHandle::clear_halt
- DeviceHandle::detach_kernel_driver
- DeviceHandle::attach_kernel_driver
- DeviceHandle::set_auto_detach_kernel_driver
- DeviceHandle::claim_interface
- DeviceHandle::release_interface
- DeviceHandle::set_alternate_setting

This is safe to do because libusb is thread-safe.

Fixes a1ien#148
surban added a commit to surban/rusb that referenced this issue Nov 13, 2023
Change the following methods to accept a non-exclusive reference:
- DeviceHandle::set_active_configuration
- DeviceHandle::unconfigure
- DeviceHandle::reset
- DeviceHandle::clear_halt
- DeviceHandle::detach_kernel_driver
- DeviceHandle::attach_kernel_driver
- DeviceHandle::set_auto_detach_kernel_driver
- DeviceHandle::claim_interface
- DeviceHandle::release_interface
- DeviceHandle::set_alternate_setting

This is safe to do because libusb is thread-safe.

Fixes a1ien#148
@a1ien a1ien closed this as completed in #186 Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants