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

Cron job doesn't get started on time #385

Closed
SimranBindra opened this issue Oct 26, 2018 · 9 comments
Closed

Cron job doesn't get started on time #385

SimranBindra opened this issue Oct 26, 2018 · 9 comments

Comments

@SimranBindra
Copy link

SimranBindra commented Oct 26, 2018

Hi,
I have a cron job defined in this way:

let job = new CronJob('0 * * * * *' , function () {--some code--}, null, true, null);

This should run the job every minute. When I start and print job.nextDate(), it prints that the job will run on "Wed Oct 31 2018 00:00:00 GMT+0530" which is 5 days away (correct timezone though)
I printed job.nextDates(5), it shows that the gap between the jobs in correct, ie, they will run every minute, but the catch remains that it will kick off only on "Wed Oct 31 2018 00:00:00 GMT+0530".
What I'm not able to understand is that how does it get this offset of 5 days.

I checked my system date and it's alright and I can't think of any other way to debug this.

Thanks

@pdspicer
Copy link

Just experienced this as well, for me it was actually "Wed Oct 31 2018 00:00:00 UTC" but close enough... only discernible difference between a system where it was working properly and one where it wasn't was patch version: working one had 1.4.1, broken one had 1.4.0. Anyways updating to 1.4.1 fixed the issue for me but I also have no explanation as to why, this seemingly just stopped working out of the blue, independent of running any kind of updates.

@ncb000gt
Copy link
Member

Hey, I put up an example of getting the next dates. I tried it locally and it seemed to work fine for me. I'm in EST (America/New_York). Can you pull this down and try again on your systems? 3020d15

Also, we just pushed a change for 1.5.0 up to NPM. That takes care of likely a different issue, but depending on your TZ you may want to take a look (probably wont affect the UTC case, but I included that in the example I pushed).

For reference, today is the 29th. ;D

node examples/get_next_runs.js
System TZ next 5:  [ moment("2018-10-29T15:03:00.000"),
  moment("2018-10-29T15:04:00.000"),
  moment("2018-10-29T15:05:00.000"),
  moment("2018-10-29T15:06:00.000"),
  moment("2018-10-29T15:07:00.000") ]
UTC next 5:  [ moment.utc("2018-10-29T19:03:00.000+00:00"),
  moment.utc("2018-10-29T19:04:00.000+00:00"),
  moment.utc("2018-10-29T19:05:00.000+00:00"),
  moment.utc("2018-10-29T19:06:00.000+00:00"),
  moment.utc("2018-10-29T19:07:00.000+00:00") ]

@pdspicer
Copy link

pdspicer commented Oct 29, 2018

@ncb000gt same behavior. Just dug in a little further, it's related to _verifyParse removing entries from daysOfMonth. This appears to have been updated in 1.4.1 so I guess the update really was the fix.

Setting breakpoints in _verifyParse I observed that at least 1 day was removed for every month with less than 31 days, but it always kept 31 in the list. I'm guessing off-by-one error in the indexing from Object.keys(this.dayOfMonth)), but I'm not as familiar with this code. End result is that if it tries to parse next run in the last 5 days of the month, it forces it to go to next month.

The effect of this removal is that the start date will be set to the 31st of the month on every job scheduled to start immediately, but only when you test in the last 5 days of the month :)

I believe the fix in 1.4.1 was ultimately a change to use the value from the key (dom) rather than the key itself (j) which avoids the off by one error. Still a little unclear about what ultimately this code is meant to accomplish, @ncb000gt if you have a few and wouldn't mind enlightening me i'd appreciate it (just for my own interest), if not then let me just say thanks for getting back to this quickly and personally I'd consider this resolved due to the fact that it is working in later versions.

@ncb000gt
Copy link
Member

Interesting. I'll take a look, but if that were the case then it should have also happened for me in my run above. I'll see what I can figure out. Thanks.

@ncb000gt
Copy link
Member

ncb000gt commented Oct 29, 2018

Alright. I noticed this behavior when I used Asia/Kolkata for the timezone. I'll dig around and see what I can come up with.

System TZ next 5:  [ moment.parseZone("2018-10-30T02:15:00.000+05:30"),
  moment.parseZone("2018-10-30T02:16:00.000+05:30"),
  moment.parseZone("2018-10-30T02:17:00.000+05:30"),
  moment.parseZone("2018-10-30T02:18:00.000+05:30"),
  moment.parseZone("2018-10-30T02:19:00.000+05:30") ]

@ncb000gt
Copy link
Member

Actually, after a bit of review, it's already the 30th in that tz so the day is actually correct there. And, to be even more clear, I checked the dsom array a few times and it was working as expected, showing all the days using the 0 * * * * * syntax. I'll keep trying to replicate it though.

@jodevsa
Copy link
Collaborator

jodevsa commented Oct 29, 2018

Hey @pdspicer ,

This bug was introduced in version 1.4.0 and later fixed in 1.4.1. we've also published version 1.5 that addresses a deadlock scenario #381

Before v 1.4.0, Function _verifyPrase was only used to detect/warn about cases were the chosen days does not exist in the chosen months. These cases result in an infinite loop while trying to detect the next run.
for example:
* * 31 1 * “At every minute on day-of-month 31 in February.”
Day 31 does not exist in February. Versions before 1.4.0 used to only warn about this. Unfortunately in cases where cron is used as a module in a large application this could be fatal, as it would block the entire event loop.

I've introduced a new strategy in v1.4.0 that mods the day with the month constraint defined in monthConstraints variable. Executing the previous cron string would have the same effect as * * * 2 1 * + the existing warning. Unfortunately the changes done introduced a new bug that was fixed in 1.4.1

@ncb000gt
Copy link
Member

@pdspicer Apparently I can't read and totally thought you said it wasn't fixed in 1.4.1. :D I'm going to close this ticket then. Thanks!

@pdspicer
Copy link

@ncb000gt thanks for the explanation and quick response!

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

4 participants