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

src: disable SetResourceConstraints with new v8 #205

Closed
wants to merge 1 commit into from
Closed

src: disable SetResourceConstraints with new v8 #205

wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link

Newer V8 versions require that resource constraints are specified
upfront, when the isolate is created. That means node-fibers is
not in a position to change the limits when it's loaded because
the isolate has already been created at that point.

The sad downside of course is that fiber stacks are now much larger
than they otherwise would be. At least it compiles with io.js now.

Fixes #203.

Newer V8 versions require that resource constraints are specified
upfront, when the isolate is created.  That means node-fibers is
not in a position to change the limits when it's loaded because
the isolate has already been created at that point.

The sad downside of course is that fiber stacks are now much larger
than they otherwise would be.  At least it compiles with io.js now.

Fixes #203.
@bnoordhuis
Copy link
Author

By the way, the test suite passes (with io.js) with this change applied.

@laverdet
Copy link
Owner

I am surprised that this works. The configured stack limit isn't a size, but is actually an address to the end of the stack. Before you had to set the limit per fiber/thread because otherwise v8 wouldn't know when to throw a RangeError. I suspect removal of this is causing some unsafe writes.

@laverdet
Copy link
Owner

Although I've contrived a test case here that works after disabling SetResourceConstraints and fails on the current npm version w/ node v0.10 (!). I don't know why it's so hard for me to get this stack guard set up correctly :'(

var Fiber = require('fibers');

// Create a bunch of fibers and consume each fiber's stack, then yield
var fibers = [];
for (var ii = 0; ii < 100; ++ii) {
    var fiber;
    fibers.push(fiber = Fiber(function() {
        var caught = false;
        var recur = 0, stop;
        function foo() {
            ++recur;
            try {
                foo();
            } catch (err) {
                if (!caught) {
                    stop = recur - 500;
                    caught = true;
                } else if (stop === recur) {
                    process.stdout.write(''); // do a thing?
                    return Fiber.yield();
                }
                --recur;
                throw err;
            }
        }
        foo();
    }));
    fiber.run();
}

// Unwind started fibers
fibers.forEach(function(fiber) {
    fiber.run();
});

console.log('pass');

@laverdet
Copy link
Owner

To be clear I wanted to respond to your comment: "The sad downside of course is that fiber stacks are now much larger than they otherwise would be."

node-fibers actually allocates the stacks upfront, outside the control of v8, see src/coroutine.cc:185 (nb: coro_stack_alloc is just a wrapper around mmap on non-Windows platforms). The ResourceConstraints API just tells v8 where the stack ends so it knows when to throw a RangeError instead of running off the bottom.

Which is why it's so confusing that not setting this limit works, because theoretically without a stack limit it would keep consuming stack space until it hits an invalid address space and dies.

@bnoordhuis
Copy link
Author

Right, I see your point. Could it be that the test suite simply doesn't exercise the stack limit enough?

FWIW, your test case passes for me with io.js. Valgrind doesn't complain either.

What is the best way forward? I suppose I could file a CR with V8 to re-add an API for setting the stack limit at run-time.

@yelworc
Copy link

yelworc commented Mar 14, 2015

@bnoordhuis did you file that CR with V8, and if so, is there a way one could show support for it? I'm afraid I can't help much in terms of development here (not my area of expertise), but I'd love to see "official" io.js compatibility in node-fibers.
FWIW, I am using your iojs-compat branch and haven't observed any new odd issues, but it's a small project with little exposure, so we're probably just not hitting the conditions that would trigger the problem described here.
Apologies for the +1 comment. Let me know if there's anything I can do to help from a library user perspective.

@bnoordhuis
Copy link
Author

I was waiting for word from @laverdet so no, I haven't done anything yet. :-)

@bnoordhuis
Copy link
Author

I was going to post that V8 has a v8::Isolate::SetStackLimit() method now but seeing the recent work in 198c2f4, I think @laverdet already found it. I'll close the PR.

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

Successfully merging this pull request may close these issues.

Build fails on io.js 1.0.2
3 participants