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

Worker disconnect is not waiting for server closing #1305

Closed
Olegas opened this issue Mar 31, 2015 · 9 comments
Closed

Worker disconnect is not waiting for server closing #1305

Olegas opened this issue Mar 31, 2015 · 9 comments
Labels
cluster Issues and PRs related to the cluster subsystem.

Comments

@Olegas
Copy link
Contributor

Olegas commented Mar 31, 2015

Demo app: https://gist.github.com/Olegas/c2e5550911cbe8b96dae

Steps to reproduce

  1. Start server
  2. Connect to it via e.g. telnet telnet localhost 8080
  3. Wait a few seconds for lines Disconnecting worker and Cluster: worker is disconnected to appear
  4. Process is not terminated because client connection is still alive
  5. Close client connection
  6. App terminates

Expected behavior:

  1. I don't expect Cluster: worker is disconnected message until all connections is closed (https://iojs.org/api/cluster.html#cluster_worker_disconnect "... wait for the 'close' event on those servers ... ")
  2. I expecting to see Server is closed message in the log (from line 32)
@mscdex mscdex added cluster Issues and PRs related to the cluster subsystem. investigate labels Mar 31, 2015
@not-implemented
Copy link
Contributor

Is this really the expected behaviour? Disconnecting the master/worker IPC channel is independent from anything else. If you want to close the server (and gracefully terminating the worker), when the master disconnects the IPC channel, you have to implement it:

cluster.worker.on('disconnect', function() {
    console.log('Worker disconnected, closing server');
    server.close();
});

var server = ...;

@Olegas
Copy link
Contributor Author

Olegas commented Apr 1, 2015

I think disconnect is independent when we talking about child and parent.

But if it is all about cluster, disconnect only then all servers is closed is very useful from my point of view.

Anyway, this behavior is described in docs. Or I get it wrong?

@Olegas
Copy link
Contributor Author

Olegas commented Apr 1, 2015

Also, in cluster, disconnect already causing servers to close. But you can't detect if all connections is done.

@not-implemented
Copy link
Contributor

Sorry, my answer was not correct - you are right, the disconnect() function closes all servers in the worker (Reading docs FTW ... ;-)).

But: In your implementation the IPC channel is really closed immediately, so the "Cluster: worker is disconnected" is correct, and the master has no chance to notice when the server is closed (except from listening to the "exit" event, when the child process exits).

The better implementation would be:

  • Do not call disconnect() from the master, instead send a custom message like worker.send('shutdown'); (as in the official example)
  • Listen on that event in the worker with process.on('message', function(msg) {if(msg === 'shutdown') {...}}); and call server.close() here
  • in server.on('close', function(){...}); call cluster.worker.disconnect() to terminate the IPC channel from the worker side - really just before exiting the worker

One more advantage with this implementation: You can send additional status messages between master and worker even while shutting down :-)

@Olegas
Copy link
Contributor Author

Olegas commented Apr 1, 2015

@not-implemented Why it is correct?

In the master, an internal message is sent to the worker causing it to call .disconnect() on itself.

I understand itself as Worker instance so...

In a worker, this function will close all servers, wait for the 'close' event on those servers, and then disconnect the IPC channel.

So I expect the following:

  1. Master on whatever reason decides to disconnect worker, it calls cluster.worker[id].disconnect()
  2. Worker internally (I mean hidden from user) receives signal and closes all servers, waits to their close callbacks
  3. When they are all done, it calls process.disconnect() internally
  4. Both, worker and master detects closed IPC and disocnnect is emitted on master, worker, and workers' process

And the things are really working this way (as seen from cluster.js implementation), besides waiting for all servers to close.

By the way, the disconnect event on worker itself is not happening (see #1304)

Concerning examples, worker.disconnect() describes another example. It demonstrates a way to stop server and, in case of long living connection, kill it on timeout. If disconnect is emitted immediately, this example makes no sense, cause' kill timeout will be immediately cleared.

@not-implemented
Copy link
Contributor

Hmm ... ok, I see :-) ... It's not implemented as documented. Any implementation like handle.on('close', ...) in cluster.js is missing, and I'll agree this makes sense after reading the docs again.

I thought I can help you here ... but I always used the cluster the other way round, and initiated the disconnect from the worker side.

@Olegas
Copy link
Contributor Author

Olegas commented Apr 9, 2015

@not-implemented
Can you look at Olegas/io.js@b96927c694786c60 and Olegas/io.js@e8ce131d73268e8c
Is it right direction?

@Olegas
Copy link
Contributor Author

Olegas commented Apr 9, 2015

@not-implemented for now, I don't know how to handle server.close() in case of round-robin distribution.

sam-github pushed a commit to sam-github/node that referenced this issue Jun 9, 2015
Before this, cluster behaves not the way it is documented.  When
disconnect is triggered, worker must wait for every server is closed
before doing disconnect actually.

Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs#1400
Fixes: nodejs#1305
@sam-github
Copy link
Contributor

fixed by #1400

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants