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.06k stars 79 forks source link

[perf] get rid of syncronous fs.statSync calls during job validation #65

Closed naz closed 2 years ago

naz commented 3 years ago

Problem

Synchronous calls are blocking the even loop leading to performance degradation:

The synchronous form blocks the Node.js event loop and further JavaScript execution until the operation is complete. (reference)

Current state

The codebase uses fs.statSync calls in Bree constructor and during job validation.

Possible solutions

There are two different parts to the problem (1) the constructor call and (2) validateJob with all it's callers (constructor and add method).

(1) Solving constructor sync calls could be approached in couple ways:

  1. Refactor constructor into 2 separate phases - general initialization and job specific initialization. Breaking down constructor into job initalization phase would allow having async job initialization logic in separate method which can be called post construction. Use could look something like this:
    const bree = new Bree({...nonJobOptions});
    bree.addJobs(jobOptionsArray, optionalRootPath);

    Main downside of this approach is not being able to construct Bree instance in one call, and makes use of root parameter somewhat ambiguous.

  2. Refactor constructor into async function. Clients could use it like this:
    const bree = await new Bree({...options});

    I'm not sure this even works, only checked briefly and that's what came up on SO. The downside of this approach is tricky use of super() when using async self invoking functions inside the constructor:

    super note: If you need to use super, you cannot call it within the async callback. (reference)

(2) As for validateJob method, I don't see a good reason not to convert it into an async function with async fs.stat call inside. Needs solving the constructor problem first as it's using validateJob internally.

Note, synchronous calls are problematic but not critical. It only effects initialization code which is rarely called. Nevertheless I think it's useful to track and solve it.

@niftylettuce @shadowgate15 would love to hear your opinion on this.

niftylettuce commented 3 years ago

Maybe we just use an event emitter instead, and fire an "error" event on bree that can be listened on, e.g. bree.once('error', err => throw err);. Bree already extends EventEmitter. PR welcome

titanism commented 2 years ago

Resolved in v9.0.0+ https://github.com/breejs/bree/releases

naz commented 2 years ago

nice 🙌