search

thegecko / webusb

Node.js implementation of the WebUSB Specification
https://thegecko.github.io/webusb/
MIT License
183 stars 27 forks source link

Separate allowed devices and connected devices. #63

Closed nodech closed 4 years ago

nodech commented 4 years ago

PR #62 introduces a bug. Allowed devices is add only and we should not remove devices from that list. But we don't want to list allowed devices as actual devices, instead we should have connected device list. (issue #55). This PR separates those two.

There is one notable assumption made with this PR, that vendorID(4 byte hex) + productID(4 byte hex) + serialNumber (stringified) can uniquely identify devices and called key. This logic is compatible with the previous logic but instead uses Maps to store connected devices and Set of those keys to list allowed devices.

UPDATE:

Another issue that #62 introduced is listening by default, which will keep the process alive. I am thinking of removing list of connected device list fully, both from usb interface and adapter.

Without listener it will go out of sync, but listeners can not be cleaned up without introducing new methods (that wont be compatible with WebUSB). So instead of keeping list, we could query list of devices every time even though it's much more expensive.

On the other side, we could have "smart" logic, to optimize to keeping lists in case there are listeners, but query slow version when there are not.

thegecko commented 4 years ago

PR #62 introduces a bug. Allowed devices is add only and we should not remove devices from that list

Could you outline the bug here? AllowedDevices aren't exposed, so I can't see where the issue is for a user.

...instead uses Maps to store connected devices and Set of those keys to list allowed devices.

Hmm, I've avoided Maps and Sets for compatibility. Introducing these would need testing on Node 6.xx. A better approach would be a solution that doesn't rely on Node.js at all (I think EventEmitter is the only Node class used currently as a simple polyfill for it exists in most environments). This should be split into a separate PR, too.

Another issue that #62 introduced is listening by default, which will keep the process alive.

Agreed, but I'd prefer to not re-list devices continuously, I've had issues with that approach in the past. Perhaps an explicit shutdown hook to disconnect the listeners would be a better approach? Again, a concern for another PR.

nodech commented 4 years ago

Could you outline the bug here? AllowedDevices aren't exposed, so I can't see where the issue is for a user.

Connected devices and allowed devices are distinct in WebUSB. Once you allow something, it should be allowed until the instance is closed. (Well, in web it's allowed indefinitely in the browser until user revokes it) BUT currently when device disconnects it will no longer be allowed (you wont get connect event) and that's not proper behavior.

This PR changes that, so allowed devices will only get appended with identifiers and connected list will be updated. (If we can address listeners problem properly or remove connected altogether with "smart" hack I mentioned to enable tracking and caching once we have at least one listener from the outside)

Hmm, I've avoided Maps and Sets for compatibility. Introducing these would need testing on Node 6.xx. A better approach would be a solution that doesn't rely on Node.js at all (I think EventEmitter is the only Node class used currently as a simple polyfill for it exists in most environments).

Well, we could use Object instead of Map or revert back to Array.

Agreed, but I'd prefer to not re-list devices continuously, I've had issues with that approach in the past. Perhaps an explicit shutdown hook to disconnect the listeners would be a better approach? Again, a concern for another PR.

This bug was introduced within same PR, that's why it ended up here.

Because I could not find proper approach for connect/disconnect,(Other than mentioned hacky way) I was thinking of introducing just a helper class DeviceManager that will solve this issues and will have consistent API for both node and the browser. (e.g. have open and close which will not do anything on brwoser, but will setup listeners on node.)

We can still implement hack whenever we have listeners to make generic getDevices faster, but without a way to clean them up it is not consistent with browser. We could probably come up Hacky shutdown code that browser API wont complain, but that does not seem right.

thegecko commented 4 years ago

@nodar-chkuaselidze thanks for your input and taking the time to explain the issue.

Apologies it's taken me a long time to get round to this, but I believe it's now fixed in master.

I took a slightly different approach, but your PR helped me to think about how to implement it and the getDevices() list now refreshes from the adapter as you suggest.