-
Notifications
You must be signed in to change notification settings - Fork 506
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
fix bug on taking control of a device: set a lock for exclusive access #650
fix bug on taking control of a device: set a lock for exclusive access #650
Conversation
Signed-off-by: Denis barbaron <[email protected]>
Signed-off-by: Denis barbaron <[email protected]>
@koral-- seems that this is still an issue. We will investigate a bit more and create follow-up ticket for this. |
@jupe, I have tested this PR extensively and haven't seen any other issues, can you describe the issue you are experiencing? Are you sure that your client side is not at fault? |
@jupe, I took a look at the code and I seem to have seen a vulnerability, I'm taking the point to investigate and fix the problem! For your part, what exactly is the problem you are experiencing? |
multiple parallel test job try to most probably this script reproduce issue: #620 |
Signed-off-by: Denis barbaron <[email protected]>
Signed-off-by: Denis barbaron <[email protected]>
Thanks for investigating this @denis99999 . I’ll test this as soon as possible. Note that I’m still using 3.7.1 release and a bit afraid to upgrade 3.7.2 since it introduces so many changes. Have to finally jump to it :) |
so far seems to work fine. One (a bit related) improvement idea for tracing - when automatic leave functionality is triggered, it doesnt trace reason which would be useful; stf/lib/units/device/plugins/group.js Line 79 in 9b8738c
|
Signed-off-by: Denis barbaron <[email protected]>
Signed-off-by: Denis barbaron <[email protected]>
Signed-off-by: Denis barbaron <[email protected]>
@jupe, the 'No longer owned..' message is normal and is traced when the device is released either by the one who previously took control of it, or by the admin, or by the device reservation mechanism who can preempt a device, we could indeed indicate the cause but what is the point of such an effort? |
it would help investigations when such issue happens. Reason would identify root cause clearly. Probably not directly related, but we have strange issue when phone disconnect from USB hub causes that also other phones adb connections break on the same host. I’ve to investigate that more, but do you have any idea what could causes that? |
@jupe ,
You can suggest a PR but honestly it doesn't seem essential to me
Yes, this looks like a hardware issue, you should at least make sure you are using a USB 2.0 hub (and not 3.x as it seems STF supports it poorly) and preferably connect it to your host's USB 2.0 port, You also need to make sure your host has enough USB channels. available |
Signed-off-by: Denis barbaron <[email protected]>
seems that latest release fixes this issue. Reproduce every time with earlier release. Didn't find any obvious reason why it happens/how it get fixed. |
@jupe , interesting, perhaps it is due to the upgrade to adbkit 3.x, I will make tests with a USB 3.x Hub to check if it we gain in stability! |
note that I did have USB 2.0 usb hub. Reproduced even with 2 phones - when pressing reboot from UI, another reserved phone connection dropped as well. |
so mystery |
It fixes the #620 issue at API level