-
Notifications
You must be signed in to change notification settings - Fork 92
Improved panic safety. #156
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
Conversation
Hidapi::new_without_enumerate().|
This PR is approaching 1 Year old! Happy birthday! But also it's time to kill it cause I don't like seeing it in my backlog. @ruabmbua Would you like me to get this into a clean state so we can merge it and be done with it Y/N? |
|
Honestly I think my response got lost in the ether, I remember I wrote something ... Glancing over it again, you made a couple of modifications to build.rs, they do not actually have anything to do with what is described in the MR? About the API change, it feels like the new API is a bit less "rust idiomatic", as it has more global state exposed in the API surface compared to before. Can we maybe resolve the panic issue while keeping the API more like before? Also, do you have any concrete examples where its preferred to not panic? In my opinion if a problem can`t be handled, its totally ok to panic in rust. Is it maybe that you consume hidapi in another library, and you do not want to have any panics reaching the user of your library? |
The issue with HidApi::new_without_enumerate() is that it makes both it and HidApi::new() potentially panic, with no way out in library contexts. Instead, on platforms that need it, the same effect can be achieved by calling HidApi::disable_device_discovery() before any HidApi contexts are created in application code. The intention is to eventually remove HidApi::new_without_enumerate().
|
Okay, since I actually got a response, I went ahead and fixed it up so we can talk about something concrete: The main point of this PR:
Housekeeping:
I'm just not gonna open one more PR that will never get merged and have to keep track off. It's the part I hate about the PR system, but I digress. Originally I had also split build.rs into a more manageable pieces rather than the current monolith - but I can't be bothered to do that again.
No. Not as long as you set global state in a C library in the backend. The global state is there in our dependencies and we can't pretend it's not. Maybe if you dive into libusb, figure out how it does device discovery and try to replicate it on the rust side? But I'm not personally interested in doing that work.
I don't have a problem with panicking. I have a problem with panicking, effectively at random, in what should be a mostly infallible function for no good reason. Imagine you're integrating a few libraries. Some use hidapi. Some libusb directly. Crappy device drivers and such. Now put yourself in the driver author's position: impl MyDriver {
fn new() -> Self {
// Do I use new() or new_without_enumerate()? I don't know.
// If another library has already been initialized before me,
// one will panic, and one won't. Which? I don't know, it's a coin's toss.
//
// Or rely on the integrator to match the version of new that is called on every single library.
// ...
// That sounds like fun!
let hidapi = HidApi::new();
// ...
}
} |
|
Thanks for revisiting the MR, I appreciate the work! |
The issue with
HidApi::new_without_enumerate()is that it makes both it andHidApi::new()potentially panic, with no way out in library contexts.Instead, on platforms that need it, the same effect can be achieved by calling the newly added
HidApi::disable_device_discovery()before any HidApi contexts are created.