FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

getDeviceList() returns stale devices by slomg · Pull Request #828 · node-usb/node-usb · GitHub

getDeviceList() returns stale devices - #828

Open
slomg wants to merge 9 commits into
node-usb:mainfrom
slomg:bug/stale-get-device-list
Open

getDeviceList() returns stale devices#828
slomg wants to merge 9 commits into
node-usb:mainfrom
slomg:bug/stale-get-device-list

Conversation

slomg commented Oct 15, 2024
edited
Loading

Copy link
Copy Markdown
Contributor

Changes

Compiling libusb without udev fixes the problem.
I tried running libusbTest from #803 (comment) and it didn't work either - unless I compiled libusb without udev.

Fixes

Checklist

Have you...

  • Tested the change acts as expected
  • Checked the change doesn't remove or change existing functionality
  • Considered how the feature impacts the product and tested around it (not just the happy paths)
  • Where possible, executed the hardware tests on multiple operating systems

Copy link
Copy Markdown
Member

Thats odd, any idea why this is the case?

slomg commented Oct 15, 2024
edited
Loading

Copy link
Copy Markdown
Contributor Author

(●__● ) great question, I have no idea
I found similar issues here: https://libusb-devel.narkive.com/pqWf4Hig/libusb-libusb-get-device-list-is-stale-and-does-not-refresh and here: libimobiledevice/usbmuxd#26

slomg commented Oct 15, 2024
edited
Loading

Copy link
Copy Markdown
Contributor Author

oops I accidently attached the wrong issue - I meant #803 haha

Copy link
Copy Markdown
Member

I think (please correct me if this is wrong):

  • udev is only used on linux (maybe mac, but certainly not windows)
  • the issue we are seeing is only on windows

Therefore, the correct fix may be to disable udev for windows builds only.

WDYT?

slomg commented Oct 15, 2024

Copy link
Copy Markdown
Contributor Author

I was running it on Linux...

Copy link
Copy Markdown
Member

I was running it on Linux...

Oh, the original issue was on windows

Copy link
Copy Markdown
Member

@JonathanLeeIFX can you confirm if this fixes your issue on Windows?

Copy link
Copy Markdown
Member

I'm not sure what the impact would be defaulting to not using udev. It seems a different source file is included:
https://github.com/node-usb/node-usb/blob/main/libusb.gypi#L90

Copy link
Copy Markdown

It's interesting that the example program did not work for you. It's supposed to be working because it only uses libusb.

This fix did not work for me, but I may have done it wrong.
My procedure:

  1. Navigate to my node-usb test project directory.
  2. Open 'node_modules/usb/binding.gyp', change line 3 to "'use_udev%': 0,".
  3. Open 'node_modules/usb/libusb.gypi', change line 3 to "'use_udev%': 0,".
  4. Recompile project with 'tsc index.ts'
  5. Run with 'node index.js'.

Output (edited for readability)
device run 1: {
...
deviceAddress: 19,
...
}

device run 2: {
...
deviceAddress: 19,
...
}
device run 3: undefined
device run 4: undefined
device run 5: {
...
deviceAddress: 19,
...
device run 6: {
...
deviceAddress: 19,
...
}

slomg commented Oct 15, 2024
edited
Loading

Copy link
Copy Markdown
Contributor Author

Did you rebuild the binaries as well? Try the prebuild script, that's what I was using when testing it

JonathanLeeIFX commented Oct 15, 2024
edited
Loading

Copy link
Copy Markdown

navigating to 'node_modules\usb' and running 'npm run prebuild' gives me the following error:

> usb@2.13.0 prebuild
> prebuildify --napi --strip --name node.napi

gyp info it worked if it ends with ok
gyp info using node-gyp@10.2.0
gyp info using node@20.15.0 | win32 | x64
gyp info find Python using Python version 3.10.0 found at "C:\Users\LeeJo\AppData\Local\Programs\Python\Python310\python.exe"

gyp http GET https://nodejs.org/dist/v23.0.0/node-v23.0.0-headers.tar.gz
gyp http 404 https://nodejs.org/dist/v23.0.0/node-v23.0.0-headers.tar.gz
gyp WARN install got an error, rolling back install
gyp ERR! configure error
gyp ERR! stack Error: 404 response downloading https://nodejs.org/dist/v23.0.0/node-v23.0.0-headers.tar.gz
gyp ERR! stack at go (C:\Users\LeeJo\Documents\Infineon\test-repository\node_modules\usb\node_modules\node-gyp\lib\install.js:223:21)
gyp ERR! stack at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
gyp ERR! stack at async install (C:\Users\LeeJo\Documents\Infineon\test-repository\node_modules\usb\node_modules\node-gyp\lib\install.js:63:18)
gyp ERR! stack at async getNodeDir (C:\Users\LeeJo\Documents\Infineon\test-repository\node_modules\usb\node_modules\node-gyp\lib\configure.js:79:7)
gyp ERR! stack at async run (C:\Users\LeeJo\Documents\Infineon\test-repository\node_modules\usb\node_modules\node-gyp\bin\node-gyp.js:81:18)
gyp ERR! System Windows_NT 10.0.19045
gyp ERR! command "c:\program files\nodejs\node.exe" "C:\Users\LeeJo\Documents\Infineon\test-repository\node_modules\usb\node_modules\node-gyp\bin\node-gyp.js" "rebuild" "--targ
et=23.0.0" "--devdir=C:\Users\LeeJo\AppData\Local\Temp\prebuildify\node" "--arch=x64" "--release"
gyp ERR! cwd C:\Users\LeeJo\Documents\Infineon\test-repository\node_modules\usb
gyp ERR! node -v v20.15.0
gyp ERR! node-gyp -v v10.2.0
gyp ERR! not ok
node-gyp exited with 1

slomg commented Oct 15, 2024
edited
Loading

Copy link
Copy Markdown
Contributor Author

hmm... Try to clone the repo, run prebuild and copy the prebuilds folder to your node_modules/usb folder

Copy link
Copy Markdown

I cloned the repo and it failed on 'npm install'. I had to copy the 'libusb' folder in from my original test repo. After that, 'npm install' worked.

I then changed 'libusb.gypi' and 'binding.gyp', ran 'npm run prebuild', and copied 'prebuilds\win32-x64' to my test repo.
No change.

slomg commented Oct 15, 2024
edited
Loading

Copy link
Copy Markdown
Contributor Author

I can't get it to work on windows either... On my Linux machine:

import { getDeviceList } from "usb";

async function main() {
    while (true) {
        const allDevices = getDeviceList();
        const device = allDevices.find(
            (device) => device.deviceDescriptor.idProduct === <PID>
        );
        console.log(device?.deviceAddress);
        await new Promise((resolve) => setTimeout(resolve, 1000));
    }
}

main();

Maybe this fix only works on linux?

Copy link
Copy Markdown

Possibly, but I'm still confused about the root issue: why does the node-usb package have an issue but not the original libusb DLL?
'getDeviceList()' should be a direct call to the DLL function. Why would it matter where it's being called from?

slomg commented Oct 15, 2024

Copy link
Copy Markdown
Contributor Author

Like I said in the PR, the original libusb DLL also didn't work for me

slomg commented Oct 16, 2024
edited
Loading

Copy link
Copy Markdown
Contributor Author

@thegecko I think I found something (I don't know c++ that well, so please correct me if I get anything wrong).
In the device constructor we are calling libusb_ref_device, which increments the ref count. Then, when we call libusb_free_device_list it only decrements it by 1, meaning it is still reffed.

If I add libusb_unref_device right after constructing the device it works, but only until the device is garbage collected in js, libusb_unref_device is called again in the deconstructor and then this assert fails.

If I remove the calls to libusb_ref_device in the constructor and libusb_unref_device in the deconstructor it works, but that's not a solution. And even after I do that is STILL doesn't work in linux unless I compile without udev

Copy link
Copy Markdown
Member

In the device constructor we are calling libusb_ref_device, which increments the ref count. Then, when we call libusb_free_device_list it only decrements it by 1, meaning it is still reffed.

I looked at this too, doesn't ref just add 1 and unref remove 1 meaning net zero overall?

slomg commented Oct 16, 2024

Copy link
Copy Markdown
Contributor Author

doesn't ref just add 1 and unref remove 1 meaning net zero overall?

it starts at 1 ref

Copy link
Copy Markdown
Member

it starts at 1 ref

then why does it get incremented in the constructor :/

slomg commented Oct 16, 2024

Copy link
Copy Markdown
Contributor Author

I don't know man, maybe it was an oversight. If I had to guess it's because when the device is destroyed and not removed from the byPtr map it can cause some very weird behaviour in the get method.

I added a log whenever a device was destroyed in the unref method, and it only ran whenever I didn't increment it in the constructor.

Copy link
Copy Markdown
Member

Thanks for your help on this, I'll have a play around. This only fixes the windows issue, right? Linux is still tied up with udev.

slomg commented Oct 17, 2024
edited
Loading

Copy link
Copy Markdown
Contributor Author

This only fixes the windows issue, right? Linux is still tied up with udev.

correct

I pushed a potential fix. If the devices we actually unrefered at the end of GetDeviceList we won't be able to use them. so instead I temporarily unrefered them before calling libusb_free_device_list, then, I set the device pointer to the new pointer when getting the device. If a device still has a nullptr libusb device, it means the device no longer exists and we can delete it.

We also can't use the pointer as the key in the byPtr map anymore as the pointer changes every time.
So instead I changed it to use the usb address. There is a small chance that 2 devices reconnect, and one of them gets the other ones address, but I couldn't think of a way to avoid it (as far as I can tell, there is no way to tell the device is the same after reconnection).

What do you think?

Copy link
Copy Markdown
Member

I changed it to use the usb address

I don't think we can rely on this, I can imagine more odd edge cases than we have at the moment :/. Perhaps we should just recreate everything as soon as a GetDeviceList is called?

Copy link
Copy Markdown
Member

Perhaps we should just recreate everything as soon as a GetDeviceList is called?

That's not going to work is it? Any existing reads/etc. would break.

I can't help feeling we are patching over and underlying issue here. We should be able to enumerate devices again while still holding references to them. What if we create a new usb context for getting a fresh list of devices then copy the changes over?

slomg commented Oct 20, 2024
edited
Loading

Copy link
Copy Markdown
Contributor Author

What if we create a new usb context for getting a fresh list of devices then copy the changes over?

How would we know what changed? how can we compare the devices and be certain they are the same physical device?

Copy link
Copy Markdown
Member

Yeah you are right.

Hmm. If we key based on libusb_get_device_address, I believe that's only unique per hub, so libusb_get_bus_number has to be in the key, too.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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 this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL