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

Use child_process.fork when running node scripts... #1134

Closed
wants to merge 1 commit into from
Closed

Use child_process.fork when running node scripts... #1134

wants to merge 1 commit into from

Conversation

heisian
Copy link
Contributor

@heisian heisian commented Nov 17, 2017

  • Handles cleanup of node child processes gracefully.
  • Doesn't pass tests, but I'm too lazy to go into this any further.
  • Regardless, child_process.fork is the correct way to handle node child processes.

@remy
Copy link
Owner

remy commented Dec 5, 2017

Thank you for this PR, but I'm 99% certain that I'm not using fork due to the way that nodemon needs to communicate (via signals and streams) with the child process. I think you can see this in the failing tests (i.e. they don't work because the code has changed to use fork).

I'm going to close for now. If you still this this is the right way for nodemon to execute the child, if you can get the tests to pass then it will help getting the change through.

@remy remy closed this Dec 5, 2017
@heisian
Copy link
Contributor Author

heisian commented Dec 6, 2017

Sure, I will look into it. Fork can communicate over stdio just as Spawn can, except fork defaults to IPC which is better for comms between node processes, IMO.

https://stackoverflow.com/a/35783167

You need to set the silent property on the options object when you pass it in to fork() in order for the stdin, stdout and stderr to get piped back to the parent process.
e.g., var n = cp.fork('./child.js', [], { silent: true });

So you or I would either need to make the fork communicate via streams, or add a whole set of conditions for when to use IPC and when to use stdio.

Do you just not have any interest in fixing this? If that's the case, I should have some time in the coming weeks b/c of the holidays.

My current workaround is that I fork a child process that has nodemon required as a module and read the child's output from the IPC channel.

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.

2 participants