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

Make module loading stack safe #145

Closed
jpolitz opened this issue Feb 10, 2014 · 4 comments
Closed

Make module loading stack safe #145

jpolitz opened this issue Feb 10, 2014 · 4 comments

Comments

@jpolitz
Copy link
Member

jpolitz commented Feb 10, 2014

Single-module execution is stack safe, but if a module blows the stack while loading, it can fail to load.

This isn't a common use case, but we need to handle it.

To handle it, we need to change the compiler to, instead of simply importing modules with:

var modname = R.getField(modImported(R, N), "provide")
... moduleBody ...

We need to do a light CPS and make this be instead:

R.safeCall(function() { return modImported(R, N); },
    function(modVal) {
       var modname = R.getField(modVal, "provide");
       ... moduleBody ...
    })
@jpolitz
Copy link
Member Author

jpolitz commented Apr 7, 2014

Closed by 7dbd5aa, before we forget about this and become baffled.

Noticed this for reals today when lowering the default gas, and pprint.arr got deep enough stacks during check-time module import to blow the stack and cause missed test results when running make compile-test.

We should move all imports over to explicitly use loadModule.

@jpolitz jpolitz closed this as completed Apr 7, 2014
@blerner
Copy link
Member

blerner commented Apr 7, 2014

Does this actually suffice? Now that this is async, presumably there's a
heavier dependence on a sufficient timeout in whatever test harness we use,
to ensure that the full control flow does eventually complete? Or am I
misunderstanding how this change works?

@jpolitz
Copy link
Member Author

jpolitz commented Apr 7, 2014

If by timeouts, you mean RequireJS timeouts, then I think this is fine. RequireJS is all about loading the compiled JS. This is for when modules get instantiated (maybe I should have said that instead of "load" in the commit message), so it happens after RJS has done its thing.

@blerner
Copy link
Member

blerner commented Apr 7, 2014

I might mean jasmine timeouts, I'm not sure. The point is that the bodies
of these modules are required for tests to complete, but will now almost
definitely run asynchronously from the invocation of the test methods by
jasmine.

Asked another way, what remaining parts of the "scaffolding" of our
runtime, tests or other infrastructure are not either already async or
cps'ed, or built to expect to work with async code? And are there any such
parts that we can't cps ourselves, for some reason? I'm just worried that
we've pushed the problem one stack frame deeper without solving it. (But
I'm tired and don't have the full safeCall calling convention in my head at
the moment... I might be worried over nothing.)

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

No branches or pull requests

2 participants