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

Enable two way communication with worker using Event Emitter pattern #210

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

S0c5
Copy link

@S0c5 S0c5 commented Nov 26, 2020

Description

This PR enables 2-way communication between the main process with the task executed in the workers using an event emitter pattern, now we can listen and emit events 🔥

function processImage(url) { // some code that process a image asyn
  return new Promise(async (resolve) => {
    this.emit('progress', { completion: 0.5 }); // the Function was overloaded with emit, on, once
    this.on('pleaseFinishTask', function() { // this Example waits for this event to complete the task
      resolve(url);
    });
    await delay(5000);
    this.emit('progress', { completion: 1 });
 });
}

const poolInstance = pool.exec(processImage, ['https://someimage.com/url'])

poolInstance.on('progress', ({  completion }) => {
  const percentage =  completion * 100;
  console.log('Image processing Completion', percentage)
  if (percentage == 1) {
    poolInstance.emit('pleaseFinishTask')
  }
});

poolInstance.then((r) => {
  console.log('Result', r);
});

Enable workers to communicate with the parent process using an EventEmitter,  now the function handlers in the workers have `this.emit` `this.on` `this.once` to handle and emit messages.
@josdejong
Copy link
Owner

Thanks David!

So this PR implements #51, right?

I will review your PR soon, all looks good and well tested at first sight.

I see the tests fail on Node 10 with the following error though, can you have a look into that?

1) Pool
       should handle crashed workers (1):
     Uncaught TypeError: Cannot set property 'killed' of null
      at ChildProcess.<anonymous> (src/WorkerHandler.js:434:30)
      at Process.ChildProcess._handle.onexit (internal/child_process.js:248:12)

See: https://travis-ci.org/github/josdejong/workerpool/jobs/746149617

@S0c5
Copy link
Author

S0c5 commented Dec 3, 2020

Hello Jos,

Yeah, this PR implements and closes the #51, I didn't have time to see what happens on node 10 but maybe I can check this weekend.

Let me know if you have comments on my code.

Regards.

@josdejong
Copy link
Owner

David I've just had a look at the API you've implemented for passing messages. Overall it looks good but I think we should align a bit more on the API.

  1. API

    The worker side now has methods on, emit, once attached to this of the function. This feels tricky to me. I would prefer a more explicit API, like the last part in this proposal of @boneskull, see Send progress/status information from worker to main process #51 (comment):

    // worker.js
    
    // worker is an EventEmitter
    let worker = workerpool.worker({
      myFunc: async () => {
    
        // emit() could attach some metadata about the worker to its payload; perhaps unique ID / pid
        worker.emit('<user-defined event name>'), data);
      }
    });

    About the main process side: I think we should bind the events to a specific task and not to the pool, like I explain in Send progress/status information from worker to main process #51 (comment):

    // main.js
    
    const result = await pool.exec('fibonacci', [10], {
      // pass an event listener along when starting a task
      on: function (event, data) {
        // event callback can be used for anything. 
        // For example event can be 'progress', and data can be a percentage like 0.25
        // maybe we should also pass the original method and arguments so you have all
        // context available
        console.log(event, data)
      }
    })

    What do you think?

  2. It is an interesting idea to also allow the main process to post messages towards the worker (with this.on('pleaseFinishTask' in your example). I guess this could be useful, but do you have concrete use cases for this? If we do not really need it, it would make the solution simpler and smaller.

  3. Maybe instead of creating an eventEmitter ourselves, we could use a small, battle-tested event emitter from npm?

  4. I do not fully understand what __EVENT__ is doing and why it is needed. Can you explain?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants