Maximum call stack size exceeded when include many templates

[hydiak]

Inspired by
#749
#643

Problem

If you include big amount of templates(~200) then precompiled template falls with error:
RangeError: Maximum call stack size exceeded
The problem is large amount of callbacks in compiled code.
It looks like this:

if (runtime.contextOrFrameLookup(context, frame, "editable")) {
                            env.getTemplate("myTemplate1", false, "templates/test.html", null, function(t_15, t_13) {
                                if (t_15) { cb(t_15);
                                    return; }
                                t_13.render(context.getVariables(), frame, function(t_16, t_14) {
                                    if (t_16) { cb(t_16);
                                        return; }
                                    output += t_14
                                    output += "\n        ";
                                    env.getTemplate("myTemplate2", false, "templates/test.html", null, function(t_19, t_17) {
                                        if (t_19) { cb(t_19);
                                            return; }
                                        t_17.render(context.getVariables(), frame, function(t_20, t_18) {
                                            if (t_20) { cb(t_20);
                                                return; }
                                            output += t_18
                                            output += "\n        ";
                                            env.getTemplate("myTemplate3", false, "templates/test.html", null, function(t_23, t_21) {
                                                if (t_23) { cb(t_23);
                                                    return; }
                                                t_21.render(context.getVariables(), frame, function(t_24, t_22) {
                                                    if (t_24) { cb(t_24);
                                                        return; }
                                                    output += t_22
                                                })
                                            })
                                        })
                                    })
                                })
                            });

So i wrote a test to reproduce this bug (look at prev. commit):

nunjucks git:(master) ✗ npm test           

> nunjucks@3.0.0-dev.3 test /Users/g.khudyakov/projects/nunjucks
> jshint . && istanbul cover ./node_modules/mocha/bin/_mocha -- -R dot tests



  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․DEBUG: compile error:  [Error: Block "test" defined more than once.]
․․․․․․․․․․DEBUG: compile error:  { [RangeError: Maximum call stack size exceeded] undefined: true }
․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․

  215 passing (4s)
  1 failing

  1) compiler should include 200 templates without call stack size exceed:
     Error: expected null to equal 'FooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \nFooInclude \n'
      at Assertion.assert (node_modules/expect.js/index.js:96:13)
      at Assertion.be.Assertion.equal (node_modules/expect.js/index.js:216:10)
      at Assertion.(anonymous function) [as be] (node_modules/expect.js/index.js:69:24)
      at equal (tests/util.js:37:24)
      at Context.<anonymous> (tests/compiler.js:940:13)
npm ERR! Test failed.  See above for more details.

Solution

Solution is to use waterfall pattern (like async-waterfall) in compiler to decrease amount of call stack size.
It runs the tasks array of functions in series, each passing their results to the next in the array.
Each task runs asynchronously even if it sync. That trick are made by calling each task in setTimout() or setImmediate().
The negative side of of this approach is its extremely low performance. Depends on browser, the function will be called in ~10 ms. Read more about it

So i decide to use sync version of waterfall https://www.npmjs.com/package/a-sync-waterfall
This version of waterfall runs tasks synchronously if it possible, otherwise asynchronously.
And you can pass forceAsync param to run all task asynchronously.

The result looks good to me. Tests are passing, my template with many includes works too.

[hydiak]

I doubt that it's good enough way to include a lib code into compiled template. Please feel free to suggest a better one

[carljm]

I don't have an issue with this if it works :-) What downside do you see that prompted this comment?

[hydiak]

It should be a static(class) method. But now it's an instance method.

[carljm]

Eh, in JS the distinction is pretty murky anyway (just a matter of this binding). Doesn't bother me.

[hydiak]

Ok then :-)

[carljm]

Thanks for looking into this! What is the impact on compiled template size? And it looks like tests are failing on some currently-supported Node versions; is this fixable?

[hydiak]

In my project i got 210 templates 530Kb uncompiled.
The impact is
922Kb vs 954Kb (~3.5% bigger)

Yeah i'll check tests

[hydiak]

Stabilised tests.

[carljm]

This is looking pretty good to me! It's a significant change, so I wouldn't mind another pair of eyes on it. @ricordisamoa @SamyPesse @oyyd ?

[ricordisamoa]

You may want to sort dependencies alphabetically

[hydiak]

Ok

[ricordisamoa]

I left a minor comment but my knowledge of Nunjucks internals is not good enough to review otherwise

[carljm]

@SamyPesse Let me know if you'll have a chance to take a look at this and test it out anytime soon. I'd like your perspective on it since you were working in the same area.

[vecmezoni]

Hey guys, any updates on this PR? We use this patch in production for month already with no issues.

[carljm]

Ok, well, it looks OK to me, and since the only available feedback is positive, merging. Thanks for the contribution, sorry for the delay!

[vecmezoni]

Thank you! 👍

[SamyPesse]

Awesome ! Thanks @hydiak 🍻

[digitaljhelms]

@carljm when will there be a patch version released to include this?

[vecmezoni]

@digitaljhelms i will publish 3.0 later this week.