LearnBoost / cluster

Node.JS multi-core server manager with plugins support.
http://learnboost.github.com/cluster
MIT License
2.29k stars 159 forks source link

Provide a mean to pass arbitrary data from worker process to master process (nice feature for standalone use-case) #92

Open Arech opened 13 years ago

Arech commented 13 years ago

Hi again)

Sometimes it is necessary to put management code into master process and collect some data from workers. This feature requires IPC link between master and worker processes. Cluster already uses such IPC link and it can be easily extended to provide necessary API for the request. Here is a quick&dirty proposition:

Update worker.js

//flag worker as started (may be not necessary and can be deduced somehow from object state, but I didn't dig so deep
this.started=true;
// emit event stating the worker has finished it's initialization
this.emit('worker started',this);
//for use in worker process
Worker.prototype.sendData=function(data){
    if (! this.started) return false;
    //console.log("#passing worker data");
    this.master.call('workerData', data);
    return true;
}

For master.js

.....
// env match
  if (this.environmentMatches) {
    // worker process
    if (this.isWorker) {
      // connect the worker
      var worker = new Worker(this);

      //bugfix 91 - set the id inside worker process
      worker.id = parseInt( process.env.CLUSTER_WORKER );
      //store worker object for later use
      this._curWorker = worker;
.....
// emit notification to master process about new data available
//for use in master process
Master.prototype.workerData = function(worker, data){
  this.emit('worker data', worker, data);
};

//set up callback on worker startup
//for use in worker process
Master.prototype.onWorkerStart=function(cb){
    if(! this.isWorker || !this._curWorker) throw new Error('Not a worker process!');
    if (this._curWorker.started) {
        cb(this._curWorker);
    }else{
        this._curWorker.on('worker started',cb);
    }
}

Now one can easily do things like this:

var cluster = require('cluster');
    , proc = cluster().start();

if (proc.isWorker) {
    var id = process.env.CLUSTER_WORKER;
    console.log('Worker process id#%d started', id);
    //without onWorkerStart can't guarantee the IPC is ready... 
    proc.onWorkerStart(function(w){
        console.log('Sending data to master process...');
        w.sendData({msg:'hello world'});
    });
    setInterval(function(){
        console.log(' processing job for worker #%d', id);
        if (Math.random()>0.8) throw new Error('test exception handling');
    }, 1000);
} else {
    console.log('Master process started');

    proc.on('worker data', function(w,data){
        console.log("Worker #%d has send message = %s",w.id, JSON.stringify(data));
    });

    setTimeout(function(){
        proc.close();
    }, 10000);
}
tj commented 13 years ago

you can use the existing internal api for this, but I dont want cluster to become an IPC library, it has nothing to do with that really aside from the fact that it needs it internally. I would recommend just using other libraries for the IPC, if nothing great exists I can tear out what I use internally and make a little local socket IPC lib or something

Arech commented 13 years ago

Motivation behind the proposition consists of the following:

//master process:
//handle worker creation
proc.on('worker', function(w){
    console.log("Worker #%d created",w.id);
    notifyMgmtServer(true,w);
});
//handle arrived data
proc.on('worker data', function(w,data){
    if (!data || !data.action) {
        log.error('WTF?! invalid package received in workerdata');
        return;
    }
    //process package
    ...
};
//handle worker suicide
proc.on('worker killed', function(w){
    log.info("Worker #"+w.id+" has been killed.");
    notifyMgmtServer(false,w);
});

Its just a logic and a common sence.

This is your library and you are the one who decides whether to evolve it or leave it as is. But if you decide not to add this feature, could you please export Worker variable from master.js exports? This won't hurt and won't offend somebody's sense of beauty, but someone may want to add more features to it's prototype...

Thanks.

tj commented 13 years ago

the argument certainly goes both ways, but im just saying that cluster is not an IPC library