RangeError: Maximum call stack size exceeded

[nin-jin]

@ChALkeR complains on subject and segfault in test/stack-overflow2.js.

Environment

Linux yoga 4.6.3-1-ARCH #1 SMP PREEMPT Fri Jun 24 21:19:13 CEST 2016 x86_64 GNU/Linux Node.js — 6.3.1

Code

const Future = require('fibers/future');

function test() {
  const future = new Future;
  setImmediate(() => future.return());
  future.wait();
}

function app() {
  const arr = new Array(3000).fill(0);
  const a = arr.map(() => Future.task(test));
  Future.wait(a);
  const b = arr.map(() => Future.task(test));
  Future.wait(b);
}

Future.task(app).resolve(error => {
  if (error) console.error(error);
});

Run

for i in `seq 100`; do node fibfail.js; done 

[ChALkeR]

Some clarification

We had a small discussion about performance, where @nin-jin provided the code similar to the above that uses node-fibers. I did not write that code nor am I even sure if it is correct — I never used node-fibers.

That code crashes for me, also test/stack-overflow2.js (from master) segfaults (which looks to be another issue, not sure why this was reported as a single one).

test/stack-overflow2.js segfaults both with Node.js from the official distribution (which is what installed by nvm, btw) and on version from Arch Linux packages. That happens both on Arch Linux (the uname above) and on Debian Jessie (VPS).

The code provided by @nin-jin fails with an error on my Arch Linux notebook both with the official binaries of Node.js 6.3.1 (link above) and the version from Arch Linux packages (also 6.3.1).

It's randomish (fails 90% of the times, depends on the Array size), so it could be a race or something like that. That doesn't happen on the Debian Jessie VPS, but that could be because that VPS is considerably slower (if this is really a race somewhere). It could happen with other numbers, though — I haven't tried good enough.

[laverdet]

The stack overflow tests included are known to fail, though the conditions created in the tests don't really mimic real-world applications. The main issue is that v8 seems to have issues with not respecting stack boundaries (see references to SetStackGuard in fibers.cc) and it's something I have never quite been able to work around. Though this actually only comes up in the case when you have runaway recursion-- instead of a thrown RangeError you get a segfault.

Support for Arch Linux (specifically musl as opposed to glibc) is somewhat experimental, so issues under extreme circumstances are not surprising. If you run into these issues I'd suggest checking out the soon-to-be-standard async+await keywords, which build upon the existing generator support in v8. The capabilities exposed by both fibers and async+await are pretty much interchangeable so migrating to one or the other shouldn't be much of a challenge.

[ChALkeR]

specifically musl as opposed to glibc

But I don't even have musl installed. The libc version used is provided by glibc 2.24-2 on my system. Perhaps you are confusing Arch with Alpine?

[ChALkeR]

I'd suggest checking out the soon-to-be-standard async+await keywords, which build upon the existing generator support in v8

That's pretty much the same what I told to @nin-jin :wink:.

[laverdet]

@ChALkeR you're right, I was confusing Arch and Alpine. Looking at @nin-jin's case a little more closely I understand why it's failing. Calling Future.wait() manually with huge amount of futures isn't really normal. There, we're getting a graceful RangeError because it's recursing too deep. I believe that since all the futures resolve immediately Future.wait() calls them all one after another and it hits the recursion limit.

You could rewrite that as follows without any issue:

"use strict"
const Future = require('./future');

function test() {
  const future = new Future;
  setImmediate(() => future.return());
  future.wait();
}

Future.task(function() {
  const arr = new Array(3000).fill(0);
  const a = arr.map(() => Future.task(test));
  a.forEach((future) => future.wait());
  const b = arr.map(() => Future.task(test));
  b.forEach((future) => future.wait());
}).detach();

The .detach() change isn't important for the fix, it's just a convenience method that will throw uncaught exceptions to the main node loop, triggering process.on('uncaughtException').

I'll see about adding a step to Future.wait() to tune back the recursion so it doesn't run into this.

Regard async+await, for new projects I think it's the right choice to use those new language features over Fibers. Future.fromPromise and future.promise() were specifically added to aid migration. I started fibers over 5 years ago back when generators were barely on the drawing board and I couldn't wait for ECMAScript to catch up. Not that I won't continue supporting fibers for current users :)

[nin-jin]

There, we're getting a graceful RangeError because it's recursing too deep.

Why theare are no errors on Windows10@64?

Regard async+await, for new projects I think it's the right choice to use those new language features over Fibers.

Epic fail. See comparison: https://github.com/nin-jin/async-js

[ChALkeR]

@laverdet But I believe this specific issue in fact could be fixed on the fibers side without changing anything on the user side, couldn't it? I.e. change Future.wait impl, perhaps splitting the array into chunks.

@nin-jin

Epic fail. See comparison

Your comparison is inaccurate, you measure a single function call of a few ms. That dousn't show anything. Also, you are missing that native async/await would be shipped relatively soon. Let's not have this discussion here, though.

[nin-jin]

@ChALkeR, not only performance i was compared, you known.