option to wait for job completion before executing the next one

[Ocaenyth]

This can be useful in cases where two ticks running at the same time could lead to conflicts, and while it's not supposed to happen, maybe some undetected errors/bugs in the onTick call might lead to one tick being too slow and having multiple ticks running at the same time

Considering you already have the job.running boolean, I think it could be a great addition whilst not being too hard, to add a new parameter that could prevent the job to fire a new tick, if one is already running.
I gave a look at the code, and figured that I misunderstood how running was working: It determines whether the job is running, not if one tick is running. Regardless of that, I still think this could be a nice feature, though I see that it is harder to implement now

Related

• Wait for job finished to execute the next one #347
• Prevent new jobs running if job still in progress nestjs/schedule#1296
• option to wait for callbacks completion on job.stop() #713

[Ocaenyth]

For anyone interested, here's the work-around that I used
Caveats : I'm developing in TypeScript, and I only have one callback for onTick, though it could be adapted to work with multiple callbacks as well

    private async fireTick() {
        if(this.isRunning) {
            this.logger.info('This CRON is already in execution');
            return;
        }
        this.isRunning = true;
        try {
            // Call your onTick callback here
            await this.myOnTickCallback();
        } catch(exception) {
            this.logger.error('Tick stopped due to an error');
            this.logger.error(exception);
        }
        this.isRunning = false;
    }

And here is how I would start this CRON

    public start() {
        const intervalString = `*/5 * * * * *`;
        const options: CronJobParameters = {
            cronTime: intervalString,
            onTick: () => this.fireTick(),
            runOnInit: true,
        };
        this.cronJob = new CronJob(options);

        this.cronJob.start();
    }

Note: start, fireTick and myOnTickCallBack are all methods of the same class

[vitorbertolucci]

That is exactly how I do it, but I eventually run into a situation where the job stops completely because isRunning never goes false, even though it is set to false on a finally case of my try / catch. It is really rare and probably related to some issue on the machine that runs the code.

In any case it would be great if there were a solution for this directly on the library both for preventing code repetition with this whole structure on every job and to natively prevent conflicts.

[intcreator]

feel free to make a PR! I'm pretty busy so it might be a while before I get to this one

[sheerlox]

drawing inspiration from croner, I suggest:

• a public property on CronJob that holds: the last tick date of the job (DateTime) + if that tick is still running (boolean)
• a parameter that accepts boolean | (job: CronJob<C>) => void (name TBD), if provided we block execution of new tick when previous while previous one is still running, and if it is a callback we call it with the current job to let users handle the information as they wish

[intcreator]

hmm that croner link isn't working for me @sheerlox 😕

is the idea that we store the tick of the last execution to compare it with something? otherwise we can just store the boolean of whether the job is still running. also we have lastDate already, would the new public property be different?

[sheerlox]

@intcreator link updated with GitHub blob permalink

[sheerlox]

lastExecution indeed already does the job! the goal is to be able to use it to tell which tick is still in progress, e.g. to determine if the tick is taking way longer than expected.

[intcreator]

oh so in the case that ticks take different times, maybe a tick other than the one from lastExecution is still running? but would we update lastExecution if a particular tick was blocked from actually executing?

[sheerlox]

lastExecution would not be updated if we block the tick, so its value corresponds to the last time we ran a tick.
If the option to prevent new ticks from overlapping is not set, then we keep the same behavior we currently have (update lastExecution and run a new tick even tho the last one might not have finished its job).

Note: I quickly checked how lastExecution works, and I can't understand why it's set on line 227 instead of line 239 🤔

EDIT: lastExecution doesn't seem to be about the last time we ran the onTick function, but about the last time we checked if we should run it (which should be different only when the time between executions is > ~24 days). I'm trying to write a test case to investigate that.

[sheerlox]

@intcreator yeah, I was correct... opened an issue: #710

[intcreator]

ok sounds good

also it's unfortunate that internally it's called lastExecution but the API function is called lastDate since those are different things. I would expect lastExecution to only update if the callback was actually executed but I would expect lastDate to update even if the callback was not executed...not sure what to do about that though

[sheerlox]

from docs: "lastDate(): Tells you the last execution date", so although the names are really confusing, I'm proposing we:

• fix issue lastDate() value for cron intervals > 25 days is sometimes wrong #710,
• deprecate lastDate() (until the next major release) & add a lastExecution() function to improve naming consistency

wdyt?

[intcreator]

yep, sounds good to me

[sheerlox]

blocked by #711. once merged this is ready for implementation :)

[sheerlox]

related to #713 since both features need to watch for callbacks completion

[konradkevin]

i put my 2 cents here sharing my own workaround
this is a simple util function that wraps the job task inside a JS closure and maintains a "isRunning" flag per job

const runUnique = (task: () => Promise<unknown>) => {
  let isRunning = false

  return async () => {
    if (isRunning) {
      console.debug('[Cron] job is already running')
      return
    }

    try {
      isRunning = true
      await task()
    } catch (e) {
      console.error('[Cron] error during job execution', e)
    } finally {
      isRunning = false
    }
  }
}

usage

CronJob.from({
  start: true,
  cronTime: '*/1 * * * *',
  onTick: runUnique(async () => {
    await myAsyncTask()
  }),
})

full demo snippet

import { CronJob } from 'cron'

let taskId = 0
const EVERY_SECOND = '* * * * * *'

function sleep(milliseconds) {
  return new Promise((resolve) => setTimeout(resolve, milliseconds))
}


const runUnique = (task) => {
  let isRunning = false

  return async () => {
    if (isRunning) {
      console.debug('[Cron] job is already running')
      return
    }

    try {
      isRunning = true
      await task()
    } catch (e) {
      console.error('[Cron] error during job execution', e)
    } finally {
      isRunning = false
    }
  }
}


async function longTask(id) {
  console.log(`start long task ${id}`)
  console.time(`end long task ${id}`)
  await sleep(5 * 1000) // sleep 5 seconds
  console.timeEnd(`end long task ${id}`)
}

CronJob.from({
  start: true,
  cronTime: EVERY_SECOND,
  onTick: runUnique(async () => {
    await longTask(++taskId)
  }),
})

[sheerlox]

Great workaround @konradkevin, thanks for sharing!