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

deadlock #381

Closed
fabiooshiro opened this issue Oct 5, 2018 · 10 comments
Closed

deadlock #381

fabiooshiro opened this issue Oct 5, 2018 · 10 comments

Comments

@fabiooshiro
Copy link

fabiooshiro commented Oct 5, 2018

console.log('fist')
new CronJob('0 0 9 4 * *', (function() {
  console.log('message');
}), null, true, 'America/Sao_Paulo');
console.log('second')

output
first

@ncb000gt
Copy link
Member

ncb000gt commented Oct 5, 2018

Are you seeing this if you take out the DoM specificity? What about if you remove the timezone?

Have you been able to pinpoint anything inside the library as the culprit?

@ncb000gt
Copy link
Member

ncb000gt commented Oct 5, 2018

I'm able to reproduce this. Thanks.

ncb000gt added a commit that referenced this issue Oct 5, 2018
When using the Sao Paulo timezone, it appears that going into the 4th of
a month only 23 hours are added to the 'day' instead of 24. This appears
to be a bug in moment-tz. For now, this commit will fix the issue until
we can resolve the larger issue.

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

ncb000gt commented Oct 5, 2018

@fabiooshiro we looked into this (mostly @jodevsa) and found some strange behavior when adding 1d and then setting the hours/minutes/seconds to 0. It appears as though only 23 hours are being added going into the 4th of the month.

I pushed a change that should help with the deadlock until we can figure out the larger resolving solution. Let me know if this works. @jodevsa is going to link to a ticket he's going to create over in the moment-tz project.

@ncb000gt
Copy link
Member

ncb000gt commented Oct 5, 2018

The tests appear to be failing, but not individually. I'll look into these later when I get a moment.

@Roger13
Copy link

Roger13 commented Oct 24, 2018

I'm still having this issue. Do we have a fix?

@ncb000gt
Copy link
Member

ncb000gt commented Oct 24, 2018

We should have commented here, sorry about that! We pushed a couple changes to fix the specific problem outlined above, and to update the tests. But, the fix we put in place was specifically for Sao_Paulo.

@Roger13 did you pull the latest from npm? can you double check the cron package.json in your node_modules directory? Also, are you using a timezone and if so, what is it? Any code you can share would be helpful too.

thanks!

@Roger13
Copy link

Roger13 commented Oct 24, 2018

Trying to run the following snippet, also using Sao_Paulo timezone:

console.log('start')
const reportCron = new CronJob('0 0 0 5,15 * *', () => console.log('Running'), null, true, 'America/Sao_Paulo')
console.log('finish')

Output: start

@ncb000gt Just ran a npm update, still having the issue, currently running cron@1.4.1

@ncb000gt
Copy link
Member

@Roger13 sorry, I edited the comment above. Can you share any more? Thanks!

@Roger13
Copy link

Roger13 commented Oct 29, 2018

@ncb000gt I don't know if you saw the snipped I shared on the comment above. Any update?

@ncb000gt
Copy link
Member

@Roger13 hey, turns out we didn't publish the code to npm, sorry about this, slight miscomm. I've published 1.5.0 to npm. Pull that down and try it again. Let me know if you're still seeing this issue. Sorry about this and thanks!

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