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

SchedulingDefault_spec.js fails with Node v6.8.0 (setImmediate problem) #7555

Closed
kirrg001 opened this issue Oct 12, 2016 · 10 comments
Closed
Assignees
Labels
bug [triage] something behaving unexpectedly server / core Issues relating to the server or core of Ghost

Comments

@kirrg001
Copy link
Contributor

kirrg001 commented Oct 12, 2016

We suddenly have a broken build https://travis-ci.org/TryGhost/Ghost/jobs/167170509.

grunt test:core/test/unit/scheduling/SchedulingDefault_spec.js fails on Node v6.8.0

The last job never get's executed, because setImmediate does not get triggered!

@kirrg001 kirrg001 added bug [triage] something behaving unexpectedly server / core Issues relating to the server or core of Ghost tests labels Oct 12, 2016
@kirrg001
Copy link
Contributor Author

@kirrg001
Copy link
Contributor Author

weird weird.

When i decrease the timeout diff from diff - 200 to for example diff - 70, then it works.
diff is the difference between the next job and now. And 200 tells the timer to wake up 200ms before, because setTimeout is not accurate.

So if the timeout get's invoked earlier to now, then it works?

https://github.com/TryGhost/Ghost/blob/master/core/server/scheduling/SchedulingDefault.js#L173

kirrg001 added a commit to kirrg001/Ghost that referenced this issue Oct 12, 2016
refs TryGhost#7555
- temporary fix to make travis green
- that should not have a any bad effect on scheduling
- we just let the job awake a bit later
- the job logic is strong enough to catch the job if setTimeout awakes too late (that can happen, because setTimeout is not accurate)
- if (moment().diff(moment(Number(timestamp))) <= self.beforePingInMs) --> is smaller ensures that even if the diff is negative, it get's executed
@kirrg001 kirrg001 changed the title SchedulingDefault_spec.js fails with Node v6.8.0 SchedulingDefault_spec.js fails with Node v6.8.0 (setImmediate problem) Oct 12, 2016
ErisDS pushed a commit that referenced this issue Oct 13, 2016
refs #7555
- temporary fix to make travis green
- that should not have a any bad effect on scheduling
- we just let the job awake a bit later
- the job logic is strong enough to catch the job if setTimeout awakes too late (that can happen, because setTimeout is not accurate)
- if (moment().diff(moment(Number(timestamp))) <= self.beforePingInMs) --> is smaller ensures that even if the diff is negative, it get's executed
@marcuspoehls
Copy link

We’ve noticed the same kind of issue with post scheduling: if there are at least 3 schedules, the scheduler won’t publish (at least) the last one.

Used Setup

  • Node.js: 4.5.0
  • Ghost: 0.11.1
  • Database: MySQL
  • Steps to reproduce: local Ghost setup with post scheduling available, requires Ghost 0.9.0 or later. Then create 3 posts, schedule them with 11 minutes time difference and let the scheduler publish them. In our setup, the scheduler didn’t recognize the third post.

@kirrg001 kirrg001 added the LTS label Oct 13, 2016
@kirrg001 kirrg001 self-assigned this Oct 13, 2016
@kirrg001
Copy link
Contributor Author

@marcuspoehls I can't reproduce what you are describing.

In our setup, the scheduler didn’t recognize the third post.

What do you mean by saying "didn’t recognize the third post"?
So you schedule the third post and then it remains in status "scheduled"?

@Fishrock123
Copy link

Node issue at nodejs/node#9084

@kirrg001
Copy link
Contributor Author

@marcuspoehls Can you please contact me in our slack channel?
I am curious why you have problems on node 4.x.x 👍
This issue here describes a problem with node 6.x.x!

@marcuspoehls
Copy link

@kirrg001 Hi Katharina, sorry for the confusion. I found the actual issue: Ghost doesn’t pick up scheduled posts correctly after restarting the process.

Looks like while testing and reproducing the behaviour I didn’t let the process run continuously.

The init method of post scheduling loads already scheduled posts with the following filters:

  • status:scheduled
  • created_at:>=moment().subtract(7, 'days').startOf('day').toDate()
  • created_at:<=moment().endOf('day').toDate()

That results in a options object like this:

{ context: { internal: true },
  from: Fri Oct 07 2016 02:00:00 GMT+0200 (CEST),
  to: Sat Oct 15 2016 01:59:59 GMT+0200 (CEST),
  filter: 'status:scheduled',
  columns: [ 'id', 'published_at', 'created_at' ] }

If there are already scheduled posts, they need to be created in between the from and to date range! Every earlier created post than the from date won’t get selected and never published through the scheduler.

Looks like the filter on from and to isn’t required at all?

@kirrg001
Copy link
Contributor Author

@marcuspoehls yeah we can kill from/to filters and just fetch all scheduled posts you have left in the database 👍

kirrg001 added a commit to kirrg001/Ghost that referenced this issue Oct 14, 2016
refs TryGhost#7555
- remove filters, they can cause problems
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Oct 14, 2016
refs TryGhost#7555
- remove filters, they can cause problems
ErisDS pushed a commit that referenced this issue Oct 14, 2016
refs #7555
- remove filters, they can cause problems
ErisDS pushed a commit that referenced this issue Oct 14, 2016
refs #7555
- remove filters, they can cause problems
@kirrg001
Copy link
Contributor Author

Node 6.8.1 was released containing the fix.
https://github.com/nodejs/node/blob/v6.8.1/doc/changelogs/CHANGELOG_V6.md#6.8.1
Will test later and then close this issue 👍

@kirrg001
Copy link
Contributor Author

It's fixed in 6.8.1!

mixonic pushed a commit to mixonic/Ghost that referenced this issue Oct 28, 2016
refs TryGhost#7555
- temporary fix to make travis green
- that should not have a any bad effect on scheduling
- we just let the job awake a bit later
- the job logic is strong enough to catch the job if setTimeout awakes too late (that can happen, because setTimeout is not accurate)
- if (moment().diff(moment(Number(timestamp))) <= self.beforePingInMs) --> is smaller ensures that even if the diff is negative, it get's executed
mixonic pushed a commit to mixonic/Ghost that referenced this issue Oct 28, 2016
geekhuyang pushed a commit to geekhuyang/Ghost that referenced this issue Nov 20, 2016
refs TryGhost#7555
- temporary fix to make travis green
- that should not have a any bad effect on scheduling
- we just let the job awake a bit later
- the job logic is strong enough to catch the job if setTimeout awakes too late (that can happen, because setTimeout is not accurate)
- if (moment().diff(moment(Number(timestamp))) <= self.beforePingInMs) --> is smaller ensures that even if the diff is negative, it get's executed
geekhuyang pushed a commit to geekhuyang/Ghost that referenced this issue Nov 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug [triage] something behaving unexpectedly server / core Issues relating to the server or core of Ghost
Projects
None yet
Development

No branches or pull requests

3 participants