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.

Sign up for GitHub

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

Jump to bottom

cluster: dont rely fork on this #5216

Closed
wants to merge 2 commits into from
Closed

cluster: dont rely fork on this #5216

wants to merge 2 commits into from

Conversation

igorklopov
Copy link
Contributor

Avoid using this in fork function to be able to run just fork() instead of cluster.fork()

@r-52 r-52 added the cluster Issues and PRs related to the cluster subsystem. label Feb 13, 2016
@mscdex
Copy link
Contributor

mscdex commented Feb 13, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/1653/

@cjihrig
Copy link
Contributor

cjihrig commented Feb 13, 2016

LGTM

1 similar comment
@mscdex
Copy link
Contributor

mscdex commented Feb 13, 2016

LGTM

@rvagg
Copy link
Member

rvagg commented Feb 15, 2016

Is the change to the tests to verify that this isn't used/needed inside lib/cluster.js or is the change in lib/cluster.js to make it so the test can just use fork()?

@igorklopov
Copy link
Contributor Author

First. Calling just fork instead of cluster.fork throws an error because this is undefined. All tests call cluster.fork. No test call just fork. So i picked a random test and made it call fork to verify the correctness of the fix.

@rvagg
Copy link
Member

rvagg commented Feb 15, 2016

thanks @igorklopov, perhaps insert a comment up there on the require to explain why it's being done here or else it's likely to be undone at some future point by someone cleaning up.

@jasnell
Copy link
Member

jasnell commented Feb 15, 2016

LGTM

@igorklopov
Copy link
Contributor Author

@rvagg i used blank space at right from forks and placed the comment there. ok?

@rvagg
Copy link
Member

rvagg commented Feb 16, 2016

love it, thanks @igorklopov, lgtm & landed @ e9ac83a

@rvagg rvagg closed this Feb 16, 2016
rvagg pushed a commit that referenced this pull request Feb 16, 2016
@igorklopov @rvagg
cluster: dont rely on this in fork
e9ac83a 
PR-URL: #5216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
rvagg pushed a commit that referenced this pull request Feb 18, 2016
@igorklopov @rvagg
cluster: dont rely on this in fork
efd6f68 
PR-URL: #5216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
stefanmb pushed a commit to stefanmb/node that referenced this pull request Feb 23, 2016
@igorklopov @stefanmb
cluster: dont rely on this in fork
56d37de 
PR-URL: nodejs#5216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
@igorklopov
cluster: dont rely on this in fork
46d09e3 
PR-URL: #5216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
@igorklopov
cluster: dont rely on this in fork
81d4127 
PR-URL: #5216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
@igorklopov
cluster: dont rely on this in fork
3a4d6ad 
PR-URL: #5216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cluster Issues and PRs related to the cluster subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@igorklopov @mscdex @cjihrig @rvagg @jasnell @MylesBorins @r-52