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

Windows node-hid apps don't exit if device.on('data', ...) event handler and device doesn't send data #358

Open
Newbrict opened this issue Dec 17, 2019 · 24 comments

Comments

@Newbrict
Copy link

Hi

I am have a piece of hardware which disconnects itself from usb (without physical d/c) approximately at the same time that I do handle.close() I am getting this error randomly about every 1/5th runs. I tried to google this issue but I only find non electron apps talking about heap corruption, unsure if it's related.

Environment:

Windows 10 Pro (1803, build 17134.1069).
"node-hid": "^1.1.0"
"electron": "^7.1.2",

Please provide some guidance on how to test this issue, I am having trouble reproducing it consistently

@todbot
Copy link
Contributor

todbot commented Dec 17, 2019

To remove Electron as a potential issue, can you create a Node.js program that connects to your device in the same way?

@Newbrict
Copy link
Author

I will do that, will take a couple minutes to set up though...

Also some more details, by chance I tried removing a .on("data") read that I was doing much earlier in the life of the app and I can't get the failure to occur anymore - it seems related but maybe it's not the root cause... I triple checked I am definitely closing the handle during that step

@Newbrict
Copy link
Author

// Helper to synchronize the hid reads
function sync_hid_read(handle) {
  return new Promise((resolve, reject) => {
    handle.on("data", (data) => {
      return resolve(data);
    });
    handle.on("error", (e) => {
      return reject(e);
    });
  });
}

I think I may have found the issue... I tried to write a sync helper for data.on which does give the data back but the node.js app doesn't end when I call it.

I think I have some misunderstanding on how promises work since I don't grasp why this doesn't work correctly... will try to figure it out and update here.

@Newbrict
Copy link
Author

Newbrict commented Dec 17, 2019

so even with a more simple read the node process doesn't end... any clues?

basically app.js containing:

console.log("test");

will run and print test and then the process ends.

but app.js containing:

let handle = new hid.HID(device.path);
handle.write(header);

// Wait for device to respond
let done = false;
handle.on("data", (data) => {
	console.log(data);
	done = true;
});
handle.on("error", (e) => {
	console.log(e);
	done = true;
});
while (!done) { await sleep(50);}
console.log("done");
handle.close();

will print the data variable, followed by done, but then just wait, it never ends the process...

@Newbrict
Copy link
Author

Newbrict commented Dec 17, 2019

So this seems to be causing my issue, after doing a read and a .close() subsequently opening a handle and doing some writes then .close() I get an error "Error: could not read from HID device". This error should not happen since I didn't register any reads with .on("data") or .on("error") I only called .write() then .close()

am I misusing the api? any suggestions?

My bad, this is not the case, had messed something up prior in testing which gave this issue, above issue still a mystery.

@todbot
Copy link
Contributor

todbot commented Dec 17, 2019

That code does not work stand-alone as you need to put await inside of an async function. Also sleep() is not defined.

I made some assumptions about the actual Node.js program you've created and made this gist: test-teensyrawhid-await.js The dependencies are node-hid and await-sleep (the sleep() I'm assuming you're using)

On MacOS 10.14.6 and Windows 10 Pro 1903, this program successfully exits. I do notice on Windows it takes about an extra second for it to exit.

(I'm suspecting all Windows HID close() issues as now being related to a long-standing issue with hidapi that hopefully will be fixed soon thanks to libusb/hidapi#133 being raised. Or it could be that Windows just takes about a second to teardown the HID subsystem for a closed device)

@todbot
Copy link
Contributor

todbot commented Dec 17, 2019

Also does this mean you're not longer getting the error code this issue is titled with and instead the issue is that the process isn't exiting?

@Newbrict
Copy link
Author

Newbrict commented Dec 17, 2019

@todbot sorry here is a more complete example code for me - this still fails to return in my case:

let sleep = (milliseconds) => {
  return new Promise(resolve => setTimeout(resolve, milliseconds))
}

let example = async () => {
  // Needs to be implemented to return a device from hid.devices()
  let device = get_device();
  if(device) {
    update_label(true, `Reading ${device.product}'s settings`);
    // custom header for my specific device which triggers the device to write
    // data
    let header = [0x00, 0x01];
    let handle = new hid.HID(device.path);
    handle.write(header);

    // Wait for device to respond
    let done = false;
    handle.on("data", (data) => {
      console.log(data);
      done = true;
    });
    handle.on("error", (e) => {
      console.log(e);
      done = true;
    });
    while (!done) {
      await sleep(50);
    }
    console.log("done");
    handle.close();
  }
}
example();

output:

$ node main.js
Reading Something's settings
<Buffer //////lot of data here////>
done

and waits forever after done output

I haven't been able to recreate the crash scenario so potentially it's an electron issue, the electron app does a lot more with node-hid so I don't think this example encompasses the entire complexity of the usage but it does encompass what ends up triggering the issue... any tips on getting more information out of electron?

@Newbrict
Copy link
Author

So regarding the initial issue of the crash, for whatever reason it seems by replace my frankenstein's monster edition of readSync using promises wrapped around .on("data") with the actual readSync call prevents the issue...

I believe there must be some kind of issue with .on("data") but at least there is a work around for me, should I close this issue? I can make another issue for the not exiting on .close() too if it would be helpful.

Thank you.

@todbot
Copy link
Contributor

todbot commented Dec 18, 2019

Hi,
Again that code is just a snippet and not runnable on its own (no update_label() nor get_device(). But I made a hopefully similar one for a Teensy RawHID here:
https://gist.github.com/todbot/5f1efe281235dc60973d44466b60d418
and it behaves as expected, exiting after receiving one report from the device, on both MacOS and Win10Pro.

At this point I'm not sure what the issue is you're seeing.

@Newbrict
Copy link
Author

so if you comment out the update_label line and implement get_device to return back the desired device (which is what my code does) then on my machine it doesn't exit... if I then unplug the device from usb it exits. I don't know why the behavior is different between our machines. It is not an issue for me since I switched to using readSync, but I guess it's not intended behavior - and I'm not sure why it's not reproducible on your end. Probably something to do with the device?

todbot added a commit that referenced this issue Dec 18, 2019
@todbot todbot changed the title error Command failed with exit code 3221226356. (aka 0xC0000374) Windows node-hid apps don't exit if device.on('data', ...) event handler and device doesn't send data Dec 18, 2019
@todbot
Copy link
Contributor

todbot commented Dec 18, 2019

Hi @Newbrict,
I think I may have discovered the issue you've been seeing, and why we experience different behavior. I think the situation is:

  1. The device.on('data',...) event handler is used
  2. The HID device does not send data
  3. The OS is Windows

Then the application will not exit because of a blocking hid_read(). In my case, my device sends data periodically every few seconds. I suspect yours does not.

If you have the ability to try node-hid from a github repo instead of npmjs, could you try out the latest node-hid on this repo and see if the change fixes your problem?

@Newbrict
Copy link
Author

You are correct my device sends data once. I will try to figure out how to load node-hid from github rather than npm and update here.

@todbot
Copy link
Contributor

todbot commented Dec 18, 2019

Thanks for the confirmation. In package.json, if you change your node-hid line to be:

  "node-hid": "github:node-hid/node-hid"

then remove node_modules and reinstall, you should get the github version. (now at 1.2.0 in case you want to verify)

@Newbrict
Copy link
Author

I tried that, but it seems the hidapi submodule isn't pulled?

yarn install
.....
fatal error C1083: Cannot open source file: '..\hidapi\windows\hid.c': No such file or directory [C:\Users\newbr\Desktop\Code\squeezed\node_modules\node-hid\build\hidapi.vcxproj]

@todbot
Copy link
Contributor

todbot commented Dec 18, 2019

Hmm right, hidapi is a submodule now. Not sure the nodejs way to do this but if you cd into the node-hid directory and do git submodule update --init it should pull down the hidapi dir.

@Newbrict
Copy link
Author

two issues:

seems yarn install does not setup actual git repos but copies over the files after pulling somehow, so I had to git init and some other finagling to get the submodule to pull.

and

yarn install seems to destroy & recreate the environment each time so even after all that finagling it just gets reset to the original state and fails to build with the same issue.

any tips?

@todbot
Copy link
Contributor

todbot commented Dec 19, 2019

No clue. I don't use yarn.
I guess I'll publish a new release, but that may have to wait a week or so.

@ericrwx
Copy link

ericrwx commented Jan 28, 2020

Hi @todbot - d762819 appears to work in my local tests.

Do we still plan on publishing a new 1.2.0 release soon?

@Newbrict
Copy link
Author

@todbot Hey - just wanted to bump @ericrwx question. Any plans?

@Lan-Hekary
Copy link

Hello @todbot
This Issue still exists today at v2.1.1
Any Ideas on how to solve it after migrating to NAPI ?

@todbot
Copy link
Contributor

todbot commented Dec 30, 2021

For now you should remove your device listeners before exiting.

@Lan-Hekary
Copy link

At first the issue was that the App Would not close, but then I found out that I don't call dev.close() before exiting the app.
This effectively now closes the application. but a new issue appeared.

The Issue I am Facing is currently is wired.
The Electron Application Closes but it delays for about a second and the exit code is 3221226505.

This Happens only if the Device is not streaming data to the PC, Also even if I request data from the device in the window close event of electron before app exits (to get a last response from the device).

I tried removing the Event listeners separately, no fix.

I issue now is that Electron is trying to close the main Thread, But there is an AsyncWorker for the read Event that is running, this Worker gets interrupted buy the dev.close() and the device _hid->_hidHandle goes null and hid_read_timeout return a len of 0 then SetError("could not read from HID device"); is Fired.
This segment of code is in HID.cc line 160.

The Problem somehow lies here. I am totally speculating but when I Add and Empty onError Callback to the Worker the problem goes away.
but this removes all Error Propagation to Node. (Because the moment I try to Callback the Function from within OnError it happens again)
I am trying to Track the source of the problem and I am not very Good with NAPI of Native Modules Debugging.

I will make a minimum Sample code to reproduce the bug for you. but after the weekend. and I will open a separate issue.
in the mean time if you have a theory or a test that you want me to test please tell me.

@NemoAlex
Copy link

@Lan-Hekary Hi I just meet same problem on version 2.1.2
Here is a quick workaround on Windows:

function sleep (ms) {
  return new Promise(resolve => setTimeout(resolve, ms))
}
var canQuit = false
app.on('will-quit', async (e) => {
  if (!canQuit) {
    // Tell Electron do not quit
    e.preventDefault()
    // Start to close
    device.close()
    // Wait 1000ms
    await sleep(1000)
    // Then quit agian
    canQuit = true
    app.quit()
  }
})

The app will quit after 1000ms. No crash any more.

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

No branches or pull requests

5 participants