Skip to content
This repository has been archived by the owner. It is now read-only.

Add IPC disconnect method when using fork #2591

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
3 participants
@AndreasMadsen
Copy link
Member

commented Jan 22, 2012

@indutny

This add a disconnect method to the child object (in parent) and in the process object (in child), when using .fork. It allow the child to self terminate since there is no IPC keeping it alive:

It allow the following issues to be fixed:

Please note that is related to #2426, I will update that one when this has landed, but it just made sense to split #2426 up in two pull requests.

This also solves some future issues related to cluster 2.0 (see #2038)

@piscisaureus

View changes

doc/api/child_processes.markdown Outdated

This event will emit after using the `.disconnect()` method in the parent or
in the child. After this event has emitted it is not possible to send messages
to the child or parent. An alternativ way to check if you can send messags

This comment has been minimized.

Copy link
@piscisaureus

piscisaureus Jan 24, 2012

Member

Fix typos

@piscisaureus

View changes

doc/api/child_processes.markdown Outdated
This event will emit after using the `.disconnect()` method in the parent or
in the child. After this event has emitted it is not possible to send messages
to the child or parent. An alternativ way to check if you can send messags
is to check if the `child.disconnected` property is true.

This comment has been minimized.

Copy link
@piscisaureus

piscisaureus Jan 24, 2012

Member

I don't like child.disconnected. Can we make it child.connected?

@piscisaureus

View changes

doc/api/child_processes.markdown Outdated
@@ -265,6 +272,11 @@ processes:
});


To close the IPC connection between parent and child use the `child.disconnect()`
method. This allow the child to die gracefull since there is no IPC channel

This comment has been minimized.

Copy link
@piscisaureus

piscisaureus Jan 24, 2012

Member

gracefull -> typo.

@piscisaureus

View changes

doc/api/child_processes.markdown Outdated
To close the IPC connection between parent and child use the `child.disconnect()`
method. This allow the child to die gracefull since there is no IPC channel
keeping it alive. When calling this method the `disconnect` event will emit
in both parent and child. And the `disconnected` flag will also be set to true.

This comment has been minimized.

Copy link
@piscisaureus

piscisaureus Jan 24, 2012

Member

(again) rename disconnected to connected

@piscisaureus

View changes

lib/child_process.js Outdated
@@ -154,6 +160,28 @@ function setupChannel(target, channel) {
return true;
};

target.disconnected = false;

This comment has been minimized.

Copy link
@piscisaureus

piscisaureus Jan 24, 2012

Member

rename to connected

@piscisaureus

View changes

lib/child_process.js Outdated
@@ -154,6 +160,28 @@ function setupChannel(target, channel) {
return true;
};

target.disconnected = false;
target.disconnect = function() {
// do not allow messages to be writen

This comment has been minimized.

Copy link
@piscisaureus

piscisaureus Jan 24, 2012

Member

writen: typo

@piscisaureus

View changes

lib/child_process.js Outdated
target.disconnected = true;
target._channel = null;

// if we are reading a message, lets wait to it finish

This comment has been minimized.

Copy link
@piscisaureus

piscisaureus Jan 24, 2012

Member

make this a valid english sentence

@indutny

View changes

lib/child_process.js Outdated

// if a message is being read, then wait until finished reading
if (channel.buffering) {
this.once('message', function () {

This comment has been minimized.

Copy link
@indutny

indutny Jan 24, 2012

Member

what if channel will emit internalMessage ?

@indutny

View changes

lib/child_process.js Outdated
}

channel.close();
this.emit('disconnect');

This comment has been minimized.

Copy link
@indutny

indutny Jan 24, 2012

Member

This code should be wrapped in function and reused

@indutny

View changes

lib/child_process.js Outdated

channel.close();
target.emit('disconnect');
}

This comment has been minimized.

Copy link
@indutny

indutny Jan 24, 2012

Member

that's cool, one last thing: please, move it inside .disconnect and rename it to finish, and rename disconnectFired to just fired (it should be in .disconnect scope too)

@indutny

View changes

lib/child_process.js Outdated
@@ -154,6 +158,34 @@ function setupChannel(target, channel) {
return true;
};

target.connected = true;
target.disconnect = function() {
if (this.connected === false) return;

This comment has been minimized.

Copy link
@indutny

indutny Jan 24, 2012

Member

just leave !this.connected, otherwise lgtm!

@indutny

This comment has been minimized.

Copy link
Member

commented Jan 24, 2012

lgtm!

@piscisaureus can you please put your word here?

Add disconnect method to child_process.fork()
This disconnect method allow the child to die graceful unlike
.kill() there would send a signal and stop the event loop.
This also adds a disconnect event and connect property.
@AndreasMadsen

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2012

@indutny I have made a correction 3e42012, after your final review. Described in commit.

fix buffering detection
There could be cases where a data chunk would contain a linebreak sign
followed by an unfinched message. This would result in .buffering
to be false. To fix this we detect if length of the JSONbuffer is zero.
@AndreasMadsen

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2012

@indutny Yup, old habit, fixed :)

@AndreasMadsen

This comment has been minimized.

Copy link
Member Author

commented Jan 28, 2012

@piscisaureus could you review this, it blocks my cluster work pretty badly.

@piscisaureus

This comment has been minimized.

Copy link
Member

commented Jan 30, 2012

Landed in 836344c. Thanks Andreas.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.