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

option to wait for job completion before executing the next one #556

Open
Tracked by #674
Ocaenyth opened this issue Feb 24, 2021 · 17 comments · May be fixed by #894
Open
Tracked by #674

option to wait for job completion before executing the next one #556

Ocaenyth opened this issue Feb 24, 2021 · 17 comments · May be fixed by #894
Labels
status:blocked Issue is blocked by another issue or external requirement type:feature New feature or feature improvement & requests

Comments

@Ocaenyth
Copy link

Ocaenyth commented Feb 24, 2021

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

@Ocaenyth Ocaenyth changed the title Prevent the cron Feature: Prevent the cron from creating a new tick if it's already running Feb 24, 2021
@Ocaenyth Ocaenyth changed the title Feature: Prevent the cron from creating a new tick if it's already running Feature: Prevent the cron from creating a new tick if one is already running Feb 24, 2021
@Ocaenyth
Copy link
Author

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
Copy link

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
Copy link
Collaborator

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

@sheerlox sheerlox changed the title Feature: Prevent the cron from creating a new tick if one is already running [FEATURE] option to prevent tick if one is already running Aug 15, 2023
@sheerlox sheerlox added the type:feature New feature or feature improvement & requests label Aug 15, 2023
@sheerlox sheerlox changed the title [FEATURE] option to prevent tick if one is already running [FEATURE] option to wait for job completion before executing the next one Aug 15, 2023
@sheerlox sheerlox added Type: Feature Request type:feature New feature or feature improvement & requests and removed type:feature New feature or feature improvement & requests type:feature-request labels Sep 30, 2023
@sheerlox sheerlox changed the title [FEATURE] option to wait for job completion before executing the next one option to wait for job completion before executing the next one Sep 30, 2023
@sheerlox sheerlox added the status:requirements Full requirements are not yet known, so implementation should not be started label Sep 30, 2023
@sheerlox
Copy link
Collaborator

sheerlox commented Sep 30, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

sheerlox commented Oct 2, 2023

@intcreator link updated with GitHub blob permalink

@sheerlox
Copy link
Collaborator

sheerlox commented Oct 2, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

sheerlox commented Oct 2, 2023

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
Copy link
Collaborator

sheerlox commented Oct 2, 2023

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

@intcreator
Copy link
Collaborator

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
Copy link
Collaborator

sheerlox commented Oct 2, 2023

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

wdyt?

@intcreator
Copy link
Collaborator

yep, sounds good to me

@sheerlox sheerlox added status:blocked Issue is blocked by another issue or external requirement and removed status:requirements Full requirements are not yet known, so implementation should not be started labels Oct 3, 2023
@sheerlox
Copy link
Collaborator

sheerlox commented Oct 3, 2023

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

@sheerlox
Copy link
Collaborator

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

@konradkevin
Copy link

konradkevin commented Aug 20, 2024

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
Copy link
Collaborator

Great workaround @konradkevin, thanks for sharing!

@Zamoca42 Zamoca42 linked a pull request Sep 4, 2024 that will close this issue
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:blocked Issue is blocked by another issue or external requirement type:feature New feature or feature improvement & requests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants