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

tests/stress_mt: ignore errors in threads for checking device counts #1447

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sonatique
Copy link
Contributor

This fixes an issue where an error that occurred in thread #X will create a false error message stating that thread #X+1 did not discover the correct number of devices

@tormodvolden
Copy link
Contributor

The original logic here was that if a thread failed in init, it wouldn't even proceed to to enumeration, hence the else clause. And a failed thread should not reset the last_devcount. But when later, the open/request/close was added, it could fail there even though the enumeration was fine. So your suggestion makes sense. Ideally it should not reset the last_devcount if the init or enumeration fails, only if they succeed but it is the open/request/close that fails. As long as last_devcount means "last devcount that possibly was correct"...

@mcuee
Copy link
Member

mcuee commented Jan 29, 2024

@tormodvolden

So this PR is good?

@tormodvolden
Copy link
Contributor

Is this important enough to delay the release?

Without this patch:
If an error in init, enumeration or control requests, the thread will not be checked for device count. The thread is considered failed anyway.

With this patch:
Even with an error in init, enumeration or control requests, the threads device count will be taken as the "correct baseline" for the following threads. This would be fine if the error is in the control requests, but probably not if the error is in the enumeration, then it would just make things more wrong.

So it might help in one case but may move bugs around, and it may make things worse for some other cases.

In principle, if different threads gets different counts, you cannot really tell which of them are correct or not. Even with this patch applied, it will just report that thread X+1 is wrong if it is different than thread X. That "wrong count" message must be taken with a large grain of salt anyway, it is just a quick hack.

I think stress_mt needs some serious rework. It needs to differentiate between errors in init, enumeration or control requests.
Also add a third round without the open and control requests.

Ideally it could go through all devices single-threaded and record a baseline, then run multithreaded and see if there is a difference (assuming the same devices are still around). At least for the device count. This wouldn't be difficult to add either. It would be more work to keep track of which devices can be opened and be sent control requests or not.

@sonatique
Copy link
Contributor Author

@tormodvolden I guess it is not important enough to delay a release. This PR and the other micro patches I made are just things I proposed along the way while developing and bug hunting Windows hotplug. These things made me pull my hair so many times that I decided to fix them, in the less invasive way.

They made my developer life easier these last days and I didn't see any drawback to them. Of course they are not revolutionary and are not aimed at rewriting anything from scratch.

I rebased my Windows hotplug PR on those because without them results are less reproducible or error prone, but of course we don't technically need them.

I really leave up to you whether and when you want to merge this.

Depending on the devices or drivers, open() may return
 LIBUSB_ERROR_ACCESS
 LIBUSB_ERROR_NOT_FOUND
 LIBUSB_ERROR_NOT_SUPPORTED
for devices "out of reach" to libusb.
This fixes an issue where an error that occurred in thread #X will create a false
error message stating that thread #X+1 did not discover the correct number of devices
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Examples Examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants