doxout / recluster

Node clustering library with support for zero downtime reloading
522 stars 44 forks source link

Send disconnect message to worker process. #10

Closed sandinmyjoints closed 11 years ago

sandinmyjoints commented 11 years ago

Here is the PR for #9. My only concern here is that I am sending sending a message immediately before calling disconnect. Is there a chance that disconnect will close the IPC channel before the message is sent/delivered? I don't think so, because I see this in child_process.js:

  target.disconnect = function() {
    if (!this.connected) {
      this.emit('error', new Error('IPC channel is already disconnected'));
      return;
    }

    // do not allow messages to be written
    this.connected = false;
    this._channel = null;

    var fired = false;
    function finish() {
      if (fired) return;
      fired = true;

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

    // If a message is being read, then wait for it to complete.
    if (channel.buffering) {
      this.once('message', finish);
      this.once('internalMessage', finish);

      return;
    }

    process.nextTick(finish);
  };

So I think the channel will stay open until this message is delivered (if it is the only message -- if there's more than one in flight, maybe the channel will close before any after the first are delivered?).

spion commented 11 years ago

For some reason the tests don't catch this right now, but its sometimes unsafe to use worker.send there directly - that is, when the worker completely fails to run and establish an IPC channel (e.g. syntax errors)

Because of that I wrapped worker.send in an empty try/catch block.

I also added a brief section on worker cleanup in the readme.

Also, you should now have push access to this repository: In the next couple of weeks I'm going to be busy with a particularly gnarly problem in our company's project so I might not have enough time for recluster :)

sandinmyjoints commented 11 years ago

Cool hack :) -- thanks!