breejs / bree

Bree is a Node.js and JavaScript job task scheduler with worker threads, cron, Date, and human syntax. Built for @ladjs, @forwardemail, @spamscanner, @cabinjs.
https://jobscheduler.net
MIT License
3.01k stars 78 forks source link

[feat] Add a "before worker created" event #166

Closed benjaminclot closed 2 years ago

benjaminclot commented 2 years ago

As of today, Bree.js emits two events: one after the worker has been created, the other after it has been deleted. Here's a really simple proposal in order to have another event, for example in order to allow modifications of data right before they're sent to the worker.

shadowgate15 commented 2 years ago

This wouldn't allow you to change data right before as it would create a race condition. There is a chance that the worker could be created before the event callback could be completed.

It would be better to use a plugin and modify the createWorker method to do that. Then you could guarantee that the data was modified right before the worker was created.

benjaminclot commented 2 years ago

This wouldn't allow you to change data right before as it would create a race condition. There is a chance that the worker could be created before the event callback could be completed.

Events raised by EventEmitter are synchronous so I have to disagree with your statement.

shadowgate15 commented 2 years ago

The callback can be async. I'm pretty certain it does not await the promise. You can write a test and prove me wrong. I would have asked for tests either way.

benjaminclot commented 2 years ago

The callback can be async. I'm pretty certain it does not await the promise. You can write a test and prove me wrong. I would have asked for tests either way.

They can indeed, but that's the developper's business, as it is now with how events are emitted.

benjaminclot commented 2 years ago

I added a working test case, which only works with synchronous code (but that's fine for my case). As you said, callbacks don't await promises. If that's too "hacky", I'll try and put together a plugin.

shadowgate15 commented 2 years ago

We will also need to update docs with the new event, assuming that all the tests pass.

benjaminclot commented 2 years ago

Done, the async test assumes a fail.

titanism commented 2 years ago

The test is not currently working, we haven't yet released a new version. If you'd like to fix the tests please do so.

titanism commented 2 years ago

We have reverted this merge due to failing tests. Hooks would be more appropriate than using event emitter. You can use any existing hooks library to accomplish this. Furthermore, any async code won't block the worker from being created before it completes (which is why we recommend hooks, it would less of an anti-pattern).

cage1618 commented 2 years ago

We have reverted this merge due to failing tests. Hooks would be more appropriate than using event emitter. You can use any existing hooks library to accomplish this. Furthermore, any async code won't block the worker from being created before it completes (which is why we recommend hooks, it would less of an anti-pattern).

How to use the hooks? e.g. beforeWorkerCreate, does it exist? I can't find it in the docs.

My needs is I want do something check before worker creation in multiple main thread(distributed deployment)and keep only one process is executing the job at a time。

Now,I can do it use the worker created event with the following code:

app.js

const Bree = require('bree');
const bree = new Bree({
  jobs: [{
    name: 'job',
    interval: 10000
  }],
  // another config
})

bree.on('worker created', (name) => {
  const worker = bree.workers.get(name);
  if (someCondition !== true) {
    worker.postMessage('cancel');
  }
});

bree.start();

job.js

const { parentPort } = require('worker_threads');

(async () => {
  if (parentPort) {
      await new Promise((resolve) => {
        parentPort.once('message', (message) => {
          message === 'cancel' ? process.exit(0) : resolve();
        });
      });

     // do something...
  } else {
    process.exit(0);
  }
})()

This code can achieve the desired effect, but isn't prefect: there are redundant worker creation and deletion process.

Thanks a lot!

titanism commented 2 years ago

We may add hooks feature sometime soon, which is the proper way to handle this (not via event emitters).

In the interim you could hack the internals of Bree and use any hook library.

cage1618 commented 2 years ago

We may add hooks feature sometime soon, which is the proper way to handle this (not via event emitters).

In the interim you could hack the internals of Bree and use any hook library.

Thanks reply. Well, waiting for good news