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

Date race and tasks with a date in the past #390

Closed
jevho opened this issue Nov 28, 2018 · 12 comments
Closed

Date race and tasks with a date in the past #390

jevho opened this issue Nov 28, 2018 · 12 comments

Comments

@jevho
Copy link

jevho commented Nov 28, 2018

While receiving bunch of tasks from external source, tasks with date in past should be processed as soon as possible, other tasks should be processed on time.

Issue with a date in the past can be handled with ease:

{cronTime: datetime, runOnInit: datetime <= new Date()}

But what if we run into racing conditions? What if datetime is milliseconds close in future?

@ncb000gt
Copy link
Member

ncb000gt commented Dec 4, 2018

I'm not sure I understand. You're worried about triggering a cronjob that does async work and takes longer to return/process than the next execution of the job?

@jevho
Copy link
Author

jevho commented Dec 4, 2018

I mean one-time jobs.
And by race condition i mean

  • if I do new CronJob({cronTime: '2018-12-04T15:00:00.001Z', ...})
  • at 2018-12-04T15:00:00.000Z.

Will this job be executed?

@ncb000gt
Copy link
Member

ncb000gt commented Dec 4, 2018

When the cronTime is a string we attempt to parse it as cron syntax. You'd need to pass in either a Date or moment object for the cronTime, but it should. Let us know if it doesn't, but there are tests that handle these cases.

@jevho
Copy link
Author

jevho commented Dec 4, 2018

I mean the case my one-time job is meant to start in couple milliseconds after new CronJob.
Please, have a closer look at milliseconds in cronTime of my example above.

@jevho
Copy link
Author

jevho commented Dec 4, 2018

Ok, I see.
Just tested, and it works:

new CronJob({cronTime: new Date((d = new Date()).setMilliseconds(d.getMilliseconds() + 1)), start: true, onTick: ... })

And even:

new CronJob({cronTime: new Date(), start: true, onTick: ... })

While

new CronJob({cronTime: new Date((d = new Date()).setMilliseconds(d.getMilliseconds() - 1)), start: true, onTick: ... })

never executes.

@ncb000gt
Copy link
Member

ncb000gt commented Dec 6, 2018

Ahh right. -1ms puts your execution in the past so it wouldn't work/execute. Was your original question about whether crons with dates in the past would execute? We can't instantiate a timeout on jobs in the past (-ms wont work) so we just don't. You should have seen an exception from the job with a timeout in the past.

https://github.com/kelektiv/node-cron/blob/master/lib/cron.js#L216

@jevho
Copy link
Author

jevho commented Dec 8, 2018

No, 've never seen any exceptions.

$ node
> const CronJob = require('cron').CronJob
undefined
> let j
undefined
> try {j = new CronJob({cronTime: new Date((d = new Date()).setMilliseconds(d.getMilliseconds() - 1)), start: true, onTick: function() {console.log('Fire!')} })} catch(err) {console.log(err)}
CronJob {
  context: [Circular],
  _callbacks: [ [Function: onTick] ],
  onComplete: undefined,
  cronTime: 
   { source: moment("2018-12-08T18:51:43.502"),
     second: {},
     minute: {},
     hour: {},
     dayOfMonth: {},
     month: {},
     dayOfWeek: {},
     realDate: true },
  unrefTimeout: undefined,
  runOnce: true,
  running: false }
>

ncb000gt added a commit that referenced this issue Dec 9, 2018
This fixes the exception for real dates that are in the past. A real
date is a hard JS date or moment object with a specific date and time of
execution.

Added readme section covering gotchas for ms level time ranges in real
date execution.

Signed-off-by: Nick Campbell <nicholas.j.campbell@gmail.com>
@ncb000gt
Copy link
Member

ncb000gt commented Dec 9, 2018

@jevho Take a look at the commit I just added. I got some time to look at the exception and the code was never reaching it (part of some of the frantic reworking a few months ago I think). I fixed that up and added some notes regarding the execution of some of the MS level time values since that's not going to work for most machines (ms+1 and so forth) which is why we shied away from that in the regular syntax in the first place.

Let me know if this doesn't resolve your issue and thanks for your patience!

@ncb000gt ncb000gt closed this as completed Dec 9, 2018
@jevho
Copy link
Author

jevho commented Dec 11, 2018

I've just tested your commit and got the exception as expected.
Thanks for your time!

But the issue remains unresolved.
As I tried the best of my English above, while receiving list of dates to put scheduled jobs on, issue with dates in the past can be solved easily:

{cronTime: datetime, runOnInit: datetime <= new Date()}

But this 'milliseconds gap' problem, as you stated in readme Gotchas (which I called 'date race', if it sounds correct), remains opened. Here still could exist problematic dates, which would not set runOnInit, but will produce an exception.

My suggestion, if I may, would be to introduce an additional flag for CronJob to let it execute jobs with date in the past ASAP without exception, which would cover both the 'date in the past' and 'milliseconds gap' issues.

Update. And what's more, as I see now, a new problem actually appeared:

> new CronJob({cronTime: new Date('1999-01-01 00:00:00'), start: true, runOnInit: true, onTick: () => {console.log('Fire!')} })
Fire!
Error: WARNING: Date in past. Will never be fired.

That is, now we have lost a simple solution to a 'date in the past' problem.

@anotheri
Copy link

anotheri commented Feb 4, 2019

Why we can't have date in the past?!
I had a neat solution when tasks which could be triggered manually had been initiated with past date.
Now I get Error: WARNING: Date in past. Will never be fired.
Please, fix it.

@ncb000gt
Copy link
Member

ncb000gt commented Feb 9, 2019

@anotheri Can you explain a little more about what you're trying to do with the dates in the past. I'm very reluctant to make exceptions here, but I'd like to hear more and see if there is a different approach to your problem. Thanks.

@anotheri
Copy link

@ncb000gt sure, so the approach I am using atm with v 1.5.1 of node-cron is separate node application, which handles:

  1. regular scheduled cron tasks,
  2. some kind of utility tasks/scripts which should be executed only when are triggered manually via HTTP request,
  3. and also provides the ability to run scheduled cron tasks (1) in case of any emergency ASAP manually.

so, to make sure that utility scripts (2) are never be executed automatically i've used to set the past date – it initiated task properly and also never run it w/o manual trigger.

Lmk if you need more technical details on that implementation, but i assume the idea of my use case is clear now.

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

No branches or pull requests

3 participants