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

using spawn() instead of fork() #37

Closed
wants to merge 1 commit into from
Closed

Conversation

Bartvds
Copy link
Contributor

@Bartvds Bartvds commented Jun 20, 2014

By using a detached process a quick parent doesn't prematurely kill the slow child (which would block the update from ever being completed successfully)

From the docs of spawn():

If the detached option is set, the child process will be made the leader of a new process group. This makes it possible for the child to continue running after the parent exits.

This is not the same as just calling unref() and disconnect() on a fork()

fixes #21

I've got this code in the wild for a while (as git-url dependency).

I re-tested this today: with npm version of update-notifier without the callback I never get an update: after resetting the lastUpdateCheck to zero and running the app the config the lastUpdateCheck gets updated but the update never shows up.

When I swap it for this patched version it works as expected.

by using a detached process a quick parent doesn't have to wait for a slow child
@sindresorhus
Copy link
Member

That's weird. It has worked fine for us. I tested it just a couple of weeks ago (without callback).

What kind of system and node version are you on?

Can you apply this to the rewrite branch instead? I'm currently finishing it up for a new release.

@satazor was the one that added it. he might know more.

@Bartvds
Copy link
Contributor Author

Bartvds commented Jun 20, 2014

Well, I'm on Windows, and this latest test was node v0.11.13, but earlier I had it on latest node v0.10. I've ran this code from a fork for a while (my old ticket #21 was from December).

I'll re-test this on on the rewrite branch and send a new PR on that.

@Bartvds
Copy link
Contributor Author

Bartvds commented Jun 20, 2014

I tested rewrite branch and it shows the same behaviour, and again the spawn() fix solves it on my Windows machine.

So I send #38

@satazor
Copy link
Contributor

satazor commented Jul 9, 2014

I've also tested the old code on osx and it worked great but if this patch works the same and works around windows issues, I would merge it

@sindresorhus
Copy link
Member

In theory doing detached: true on fork should work too as it's just a small wrapper upon spawn: https://github.com/joyent/node/blob/832d4db5f221705f8a1058a6aa3e4bcc9a5de8ad/lib/child_process.js#L553-L577

I guess this is fine. But can you open a ticket on node core about this? There's clearly a bug or doc issue since it works on OS X, but not on Windows for you.

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.

Usage without callback never sets notifier.update
3 participants