search

aredridel / html5

Event-driven HTML5 Parser in Javascript
http://dinhe.net/~aredridel/projects/js/html5/
MIT License
590 stars 168 forks source link

stack overflow running tests in node 0.10 #73

Closed metamatt closed 11 years ago

metamatt commented 11 years ago

I tried running tree-construction-test.js in node 0.10, and test case data/tree-construction/tests25.dat-19 failed first with a few hundred repeats of

(node) warning: Recursive process.nextTick detected. This will break in the next version of node. Please use setImmediate for recursive deferral.

and then finally

RangeError: Maximum call stack size exceeded
cscott commented 11 years ago

I know mocha had similar issues, see https://github.com/visionmedia/mocha/issues/777 -- so this might be a bug in the test runner, not in html5 itself.

metamatt commented 11 years ago

Note that html5 is using node-tap, not mocha, as its test runner.

I expect it is a similar issue where nextTick should be replaced with setImmediate, though.

I find stack overflows really painful to troubleshoot in the current Node releases because you get no context (call stack) at all. This is actually due to V8, not Node, and V8 fixed this a while ago, and I've been eagerly awaiting it showing up in Node (it showed up in V8 3.15, Node 0.10 is using V8 3.14, Node 0.11 unstable cycle releases are now at V8 3.16.17) so I built node from current 0.11 bleeding edge source the other day to try this out. For this issue I get this stack trace:

RangeError: Maximum call stack size exceeded
    at node.js
    at maxTickWarn (node.js:379:9)
    at process.nextTick (node.js:480:9)
    at onwrite (_stream_writable.js:250:15)
    at WritableState.onwrite (_stream_writable.js:92:5)
    at WriteStream.Socket._write (net.js:634:5)
    at doWrite (_stream_writable.js:211:10)
    at writeOrBuffer (_stream_writable.js:201:5)
    at WriteStream.Writable.write (_stream_writable.js:172:11)
    at WriteStream.Socket.write (net.js:596:40)

which is something, but not enough to go on. Invoking node with --throw-deprecation turns out to be more helpful, as it gives us a call stack from the first offense (when process.nextTick finds recursive invocations), not the last offense (when it's happened so many times the stack overflows). That gives us

Error: (node) warning: Recursive process.nextTick detected. This will break in the next version of node. Please use setImmediate for recursive deferral.
    at maxTickWarn (node.js:375:15)
    at process.nextTick (node.js:480:9)
    at onwrite (_stream_writable.js:250:15)
    at WritableState.onwrite (_stream_writable.js:92:5)
    at WriteStream.Socket._write (net.js:634:5)
    at doWrite (_stream_writable.js:211:10)
    at writeOrBuffer (_stream_writable.js:201:5)
    at WriteStream.Writable.write (_stream_writable.js:172:11)
    at WriteStream.Socket.write (net.js:596:40)
    at TapProducer.ondata (stream.js:51:26)
    at TapProducer.EventEmitter.emit (events.js:95:17)
    at TapProducer.write (/Users/magi/src/open/html5/node_modules/tap/lib/tap-producer.js:37:8)

Still not enough frames to see all the way from one nextTick call to the recursive one, but it does point the finger generally in tap's direction. Without taking the time to deeply understand the tap code, I found two uses of process.nextTick in its codebase proper, changed both to setImmediate, and reran the tree-construction-test.js, and it does survive the test25.dat-19 test case that stack-overflowed before.

However the tests run much more slowly, and basically tie up the CPU. So this doesn't seem like a good idea.

aredridel commented 11 years ago

Oh, excellent! Thank you!

aredridel commented 11 years ago

This should be mostly better now -- can you try?