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

Hang on Disabled Windows Notifications #334

Closed
JasonGore opened this issue Jun 16, 2020 · 5 comments · Fixed by #335
Closed

Hang on Disabled Windows Notifications #334

JasonGore opened this issue Jun 16, 2020 · 5 comments · Fixed by #335

Comments

@JasonGore
Copy link
Contributor

JasonGore commented Jun 16, 2020

Expected Behavior

Disabling Windows 10 notifications should not hang node-notifier.

Actual Behavior

Running node-notifier on Windows 10 clients with notifications results in a hang.

Repro

The simplest repro is to disable Windows 10 notifications as shown below and run npm run example in the repo.

I'm trying a scenario like:

try {
  const notifier = require('node-notifier');
  notifier.notify({
    title: 'Title',
    message: 'message',
    wait: true
  });
} catch {
  // Notifications may be disabled in OS, causing an error.
  // node-notifier may not installed.
  // Silently fail.
}

I'd just simply like to catch exceptions and silently continue.

However, if notifications are disabled on Windows 10:

image

This example will hang forever on the command line:

λ node not.js
Notifications are disabled
Reason: DisabledForUser Please make sure that the app id is set correctly.
Command Line: G:\git\ooui-dp\node_modules\node-notifier\vendor\snoreToast\snoretoast-x64.exe -pipeName \\.\pipe\notifierPipe-f80dfe8f-ff7d-42ce-becd-0a2e340a9d59 -m message -t Title

I tried searching the docs and existing issues for a way to "detect" whether notifications are disabled, but I haven't found anything. I'd also like to suppress error output like this if possible. In any case, it doesn't seem like the call to node-notifier should hang like this.

Tools

node-notifier: 7.0.1
node: 12.18.0
OS: Windows 10 Pro 1909

@peppelongo96
Copy link

You can't catch any exception due to the right execution of node-notifier. Seems the developer have choosen to print an "error" message about disabled notifications system . Also, for sure, I'm right with you that is an useless info.

@JasonGore
Copy link
Contributor Author

I've done a little more investigation. It appears in this case snoretoast never writes to pipeName arg, which means the named pipe never has a data event and therefore never has end called. This means the named pipe server keeps running which is what's causing the hang.

I have a local fix that passes back the server instance to toaster.js so that it can make sure the server is closed. I'm not sure if it's the best fix but I can make a PR.

@JasonGore
Copy link
Contributor Author

Some more findings: The error output is generated in snoretoast but bypasses standard stdio constructs entirely. It doesn't even come through on stdio via Node. It appears the reason is due to snoretoast's use of AttachConsole, which prevents this error output from feeding back into node-notifier.

@JasonGore
Copy link
Contributor Author

I've entered a snoretoast bug as well.

@JasonGore
Copy link
Contributor Author

I have a PR that doesn't totally fix the issue (due to snoretoast's behavior) but at least resolves the hanging issue. I think it's important to fix with some kind of change at least temporarily to prevent a permanent hang in certain use cases.

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 a pull request may close this issue.

2 participants