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

process._tickCallback - tock is undefined #6346

Closed
dnwe opened this issue Apr 22, 2016 · 23 comments
Closed

process._tickCallback - tock is undefined #6346

dnwe opened this issue Apr 22, 2016 · 23 comments
Labels
process Issues and PRs related to the process subsystem. v8 engine Issues and PRs related to the V8 dependency.

Comments

@dnwe
Copy link

dnwe commented Apr 22, 2016

  • Version: v4.3.1
  • Platform: Linux 4.4.0-1-amd64
  • Subsystem: process
node.js:342
          callback = tock.callback;
                         ^

TypeError: Cannot read property 'callback' of undefined
    at process._tickCallback (node.js:342:26)

TL;DR the same issue that someone else previously reported in #4308 (and closed), but with a reduced testcase.

Several of our customers are seeing this after moving from Node 0.12.x to node 4.x.x. The issue appears to be readily reproducible for me on Node 4.x.x (both 4.3.1 and 4.4.3 tested) using the example app here against an mqlight backend, but appears to be "fixed" on Node 5.10.1. Essentially, after some period of the app running, tickInfo[kLength] seems to have got out of sync and is greater-than nextTickQueue.length - hence tock ends up being undefined.

    function _tickCallback() {
      var callback, args, tock;

      do {
        while (tickInfo[kIndex] < tickInfo[kLength]) {
          tock = nextTickQueue[tickInfo[kIndex]++];
          callback = tock.callback;       // <------- line 341. tock is undefined
          args = tock.args;

Further debugging on 4.3.1 /seemed/ to indicate that the issue occurred in nextTick, after pushing the new TickObject nextTickQueue.length was 0 (descoped?), but tickInfo[kLength] was incremented regardless and hence they became out of step.

Applying this patch to src/node.js and re-building seemed to fix the issue for me...but I can't explain why.

@dnwe
Copy link
Author

dnwe commented Apr 22, 2016

@bnoordhuis - FYI, I thought I'd raise an issue for that bug we chatted about

@mscdex mscdex added the process Issues and PRs related to the process subsystem. label Apr 22, 2016
@bnoordhuis
Copy link
Member

I've not been able to reproduce the issue myself so far but I suspect a V8 bug. Do you still see the issue with node --nocrankshaft app.js?

@dnwe
Copy link
Author

dnwe commented Apr 22, 2016

$ node --nocrankshaft app.js
node.js:340
          callback = tock.callback;
                         ^

TypeError: Cannot read property 'callback' of undefined
    at process._tickCallback (node.js:340:26)

node --nocrankshaft app.js  25.50s user 2.36s system 82% cpu 33.929 total

@bnoordhuis
Copy link
Member

@nodejs/v8 Ideas? I remember there was a similar bug a few months back but the details elude me.

@indutny
Copy link
Member

indutny commented Apr 22, 2016

@dnwe may I ask you to run it with --trace-ic --nocrankshaft? It will produce tremendous amount of logs, but may help in figuring out the cause of this. Thanks!

@Fishrock123 Fishrock123 added the v8 engine Issues and PRs related to the V8 dependency. label Apr 23, 2016
@dnwe
Copy link
Author

dnwe commented Apr 23, 2016

@indutny sure, here you go trace.zip

@dnwe
Copy link
Author

dnwe commented Apr 25, 2016

@indutny @bnoordhuis : just in case it is useful, here is a xz compressed core file (3.7M) from /opt/node-v4.4.3-linux-x64/bin/node --nocrankshaft --abort-on-uncaught-exception app.js using the linux x64 release from latest-v4.x

% sha256sum /opt/node-v4.4.3-linux-x64/bin/node cf02d6764e1c48ffc0ed95a42a75d46ba1713a702e8ed83bbc3b806ee7eae54e /opt/node-v4.4.3-linux-x64/bin/node

@indutny
Copy link
Member

indutny commented May 12, 2016

@dnakamura sorry for delay! I'm trying to figure out similar bug at the moment. May I ask you to give it a spin with --no-use-ic flag as well?

@dnwe
Copy link
Author

dnwe commented May 12, 2016

@indutny sure, I'll run that for you tmrw.

We believe the underlying cause /may/ have been related to callbacks we were making to js logger from within destructors in our C++ add-on. After removing those I haven't been able to reproduce again.

@indutny
Copy link
Member

indutny commented May 12, 2016

@dnwe thank you! That's pretty interesting finding.

However similar thing appears to be happening in node-spdy, which doesn't use any native addons. Either these are two unrelated bugs, or they are a manifestation of the same bug.

@indutny
Copy link
Member

indutny commented May 12, 2016

Anyway here is a v8 bug: https://bugs.chromium.org/p/v8/issues/detail?id=5009

@hoschid
Copy link

hoschid commented Jul 4, 2016

I'm the author of #4308. I had the same problem again after i closed that issue. Compiling node with the patch described above by @dnwe solved the problem for me. But I wonder if there's any news on this issue? It seams like the related v8 bug was fixed.

@bnoordhuis
Copy link
Member

#7531 should fix this (in v6) when released, assuming v8:5009 is the root cause.

@indutny You want to take a stab at back-porting it to v4.x? It doesn't apply cleanly and I'm not sure if the changes are semantically correct for V8 4.5.

@aceway
Copy link

aceway commented Feb 23, 2017

hi, guys, what about this bug now
I encountered the problem with node v4.7.3 on debian7. The node v4.8.0(LTS) was released in the office web yesterday, was the bug fixed in the v4.8.0?

@bnoordhuis
Copy link
Member

@indutny Did you get around to back-porting the fix? Maybe I'll take a stab at it if you haven't.

I remember looking into it in July and concluding it would have to be redone from scratch for v4.x because the code bases had diverged too much.

@indutny
Copy link
Member

indutny commented Feb 23, 2017

@bnoordhuis yeah, it would be pretty hard to do it. I never got around to back-port it.

Will give it another try tomorrow, if you won't hear anything from me until Monday - feel free to take it over from me.

@bnoordhuis
Copy link
Member

@indutny Any luck? Mind if I have a go?

@aceway
Copy link

aceway commented Mar 31, 2017

Last weak, node-v4.8.1 released, but I find nothing about this bug in the commits
Would you give up to fixed this bug?

@addaleax
Copy link
Member

I’m not sure what the chances of this getting fixed are as long as there’s no reproduction or other kind of information that helps debug the problem available.

@Trott
Copy link
Member

Trott commented Aug 6, 2017

Is it correct to say that the bug has been observed in 4.x but that the most recent releases of the Node.js 6.x and 8.x lines are believed to be unaffected?

@bnoordhuis
Copy link
Member

@Trott Correct. The fix for v8:5009 is almost certainly applicable to v4.x because when you prep deps/v8/test/mjsunit/regress/regress-v8-5009.js to work inside of node instead of d8, it shows the bug (but only with v4.x, not with v6.x and v8.x.)

Back-porting is a heck of a lot of work though and will need careful review.

@maclover7 maclover7 added the v4.x label Nov 24, 2017
@apapirovski
Copy link
Member

@bnoordhuis would you like for this to remain open? v4.x is almost near EOL and it doesn't seems like there's anyone that has both the expertise and the time + willingness required to back-port this. I will close as wontfix in a week if there are no updates.

@bnoordhuis
Copy link
Member

I guess this isn't going to happen. I'll close it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests