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

doc: make socket IPC examples more robust #13196

Merged
merged 1 commit into from
Jun 22, 2017
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented May 24, 2017

This commit aims to improve the documentation examples that send sockets over IPC channels. Specifically, pauseOnConnect is added to a server that inspects the socket before sending and a
'message' handler adds a check that the socket still exists.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

R= @santigimeno

@cjihrig cjihrig requested a review from santigimeno May 24, 2017 15:04
@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels May 24, 2017
Copy link
Member

@santigimeno santigimeno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a suggestion

@@ -1136,7 +1136,7 @@ const normal = require('child_process').fork('child.js', ['normal']);
const special = require('child_process').fork('child.js', ['special']);

// Open up the server and send sockets to child
const server = require('net').createServer();
const server = require('net').createServer({ pauseOnConnect: true });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a note why using pauseOnConnect: true is a good practice.

@@ -1166,6 +1171,10 @@ tracking when the socket is destroyed. To indicate this, the `.connections`
property becomes `null`. It is recommended not to use `.maxConnections` when
this occurs.

It is also recommended that that any `'message'` handlers in the
Copy link
Member

@lpinca lpinca May 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: is that repeated on purpose?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't think so.

@addaleax
Copy link
Member

addaleax commented Jun 9, 2017

@cjihrig This would need a rebase :)

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 22, 2017

Rebased and addressed comments.

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

This commit aims to improve the documentation examples that send
sockets over IPC channels. Specifically, pauseOnConnect is added
to a server that inspects the socket before sending and a
'message' handler adds a check that the socket still exists.

PR-URL: nodejs#13196
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@cjihrig cjihrig merged commit c02dcc7 into nodejs:master Jun 22, 2017
@cjihrig cjihrig deleted the docs branch June 22, 2017 20:57
@addaleax addaleax mentioned this pull request Jun 24, 2017
addaleax pushed a commit that referenced this pull request Jun 24, 2017
This commit aims to improve the documentation examples that send
sockets over IPC channels. Specifically, pauseOnConnect is added
to a server that inspects the socket before sending and a
'message' handler adds a check that the socket still exists.

PR-URL: #13196
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@addaleax addaleax mentioned this pull request Jun 24, 2017
@MylesBorins
Copy link
Contributor

Is this applicable to v6.x?

@MylesBorins
Copy link
Contributor

ping @cjihrig

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 15, 2017

Yes, I'd say it's applicable.

@MylesBorins
Copy link
Contributor

@cjihrig do you have time to backport?

@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 20, 2017

Backport in #15482

MylesBorins pushed a commit that referenced this pull request Oct 16, 2017
This commit aims to improve the documentation examples that send
sockets over IPC channels. Specifically, pauseOnConnect is added
to a server that inspects the socket before sending and a
'message' handler adds a check that the socket still exists.

PR-URL: #13196
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 17, 2017
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
This commit aims to improve the documentation examples that send
sockets over IPC channels. Specifically, pauseOnConnect is added
to a server that inspects the socket before sending and a
'message' handler adds a check that the socket still exists.

PR-URL: #13196
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2017
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
This commit aims to improve the documentation examples that send
sockets over IPC channels. Specifically, pauseOnConnect is added
to a server that inspects the socket before sending and a
'message' handler adds a check that the socket still exists.

PR-URL: nodejs/node#13196
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants