Implement thread-safe variant of handle_events

[StephanvanSchaik]

This is the first PR split off from PR #153 that focuses on implementing a thread-safe version of handle_events() as outlined in the libusb documentation, but also it is based on the code in PR #143 with some fixes to make sure it always makes progress, even if the timeout it is 0.

libusb seems to require a timeout of at least 1ms to make any progress, so we ensure that the timeout is always at least 1ms when calling into libusb. Other than that we rely on loop and if to implement a do while loop, such that the code is executed at least once.

[kevinmehall]

How does this differ from what the call to libusb_handle_events_timeout_completed is already doing? It looks like this is basically translating the implementation of libusb_handle_events_timeout from C to Rust.

In the code I wrote as a precursor to #143, the reason for reimplementing libusb_handle_events_timeout_completed was to make the completed flag be AtomicBool rather than int, so it's not UB to write from another thread. The existing code here doesn't expose the completed arg, and this doesn't implement anything taking its place.

[StephanvanSchaik]

How does this differ from what the call to libusb_handle_events_timeout_completed is already doing? It looks like this is basically translating the implementation of libusb_handle_events_timeout from C to Rust.

Apparently, I didn't read Multi-threaded applications and asynchronous I/O carefully enough and missed the part that says libusb_handle_events_timeout_completed() does exactly this and ended up thinking I had to implement the same solution outlined there for multi-threading.

There seems to be a weird quirk in libusb where the documentation states: "If a zero timeval is passed, this function will handle any already-pending events and then immediately return in non-blocking style.", but yet if I pass a zero timeval, the function does not handle any events. It requires some non-zero timeout for it to make actual progress.

In the code I wrote as a precursor to #143, the reason for reimplementing libusb_handle_events_timeout_completed was to make the completed flag be AtomicBool rather than int, so it's not UB to write from another thread. The existing code here doesn't expose the completed arg, and this doesn't implement anything taking its place.

The motivation to use AtomicBool instead of int makes sense to me. I have mostly been trying to implement #153, which focuses on an async implementation of rusb that works well with Tokio, but ended up with the weird quirk in libusb_handle_events_timeout_completed(). I haven't specifically looked at the completed part, as I am relying on Arc<Mutex<InnerTransfer>> in #153 to mark the transfer as handled by the callback, although using completed could make the code a bit better in terms of performance/contention.

I guess that I can either use Duration::from_millis(1) in my case and close this PR to simplify things or also implement the completed argument as in PR #143, since we are already specifically talking about UsbContext::handle_events() here and use that for the rest of PR #153 as well.

[kevinmehall]

If you're trying to use it non-blocking, the completed part is probably not relevant to you, as it's just for a race-free way to break out of an event handling loop after the completion of a particular event.

[kevinmehall]

Is it possible that the 1ms timeout is just giving it time to complete within the first call to poll, and the behavior you're encountering with 0 is just the issue I reported on the other PR in which it will deadlock because nothing triggers another call to poll and handle_events?

[StephanvanSchaik]

Thank you for the very insightful comments!

Is it possible that the 1ms timeout is just giving it time to complete within the first call to poll, and the behavior you're encountering with 0 is just the issue I reported on the other PR in which it will deadlock because nothing triggers another call to poll and handle_events?

Yes, it looks like you are right. I removed this part from the poll() and instead spawned a thread to handle events in the background, and there the timeout value of 0 just works fine. I am leaning towards option 1 as you outlined in your comment in issue #153, as I do care about supporting Microsoft Windows and MacOS and it seems to be the simpler one of the two.

If you're trying to use it non-blocking, the completed part is probably not relevant to you, as it's just for a race-free way to break out of an event handling loop after the completion of a particular event.

With a background thread blocking on handling events, I probably need something similar anyway if I want to be able to quit the thread gracefully and call join() on its handle. I will look a bit more into this tomorrow.

[StephanvanSchaik]

I am closing this PR, since it looks like my current two pull requests to merge an asynchronous API for rusb do not need this bit at all.