Current version(8.0.7) make Out Of Memory

egaoneko commented 4 years ago

node.js version: 10.16.0

npm/yarn and version: 1.17.3

@koa/router version: 8.0.7

koa version: 2.11.0

Error message:

<--- Last few GCs --->

[60877:0x102b00000]    35217 ms: Mark-sweep 1387.5 (1416.2) -> 1387.5 (1416.2) MB, 1598.4 / 0.0 ms  (average mu = 0.095, current mu = 0.000) last resort GC in old space requested
[60877:0x102b00000]    36649 ms: Mark-sweep 1387.5 (1416.2) -> 1387.5 (1416.2) MB, 1432.6 / 0.0 ms  (average mu = 0.058, current mu = 0.000) last resort GC in old space requested

<--- JS stacktrace --->

==== JS stack trace =========================================

    0: ExitFrame [pc: 0x3557fc5be3d]
Security context: 0x13a08dc1e6e9 <JSObject>
    1: /* anonymous */ [0x13a0aa29e261] [/Users/user/Documents/workspace/my-project/@koa/router/lib/router.js:~246] [pc=0x3558022e30f](this=0x13a0c2a0be31 <Router map = 0x13a0a2f5da09>)
    2: arguments adaptor frame: 25->0
    3: /* anonymous */(aka /* anonymous */) [0x13a0c24028d9] [0x13a0d3c026f1 <undefined>:30] [bytecode=0x13a00c60dcd...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory
 1: 0x10003cf99 node::Abort() [/Users/user/.nvm/versions/node/v10.16.0/bin/node]
 2: 0x10003d1a3 node::OnFatalError(char const*, char const*) [/Users/user/.nvm/versions/node/v10.16.0/bin/node]
 3: 0x1001b7835 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [/Users/user/.nvm/versions/node/v10.16.0/bin/node]
 4: 0x100585682 v8::internal::Heap::FatalProcessOutOfMemory(char const*) [/Users/user/.nvm/versions/node/v10.16.0/bin/node]
 5: 0x10058eb84 v8::internal::Heap::AllocateRawWithRetryOrFail(int, v8::internal::AllocationSpace, v8::internal::AllocationAlignment) [/Users/user/.nvm/versions/node/v10.16.0/bin/node]
 6: 0x10055de86 v8::internal::Factory::NewFixedArrayWithFiller(v8::internal::Heap::RootListIndex, int, v8::internal::Object*, v8::internal::PretenureFlag) [/Users/user/.nvm/versions/node/v10.16.0/bin/node]
 7: 0x1005052ae v8::internal::(anonymous namespace)::ElementsAccessorBase<v8::internal::(anonymous namespace)::FastPackedObjectElementsAccessor, v8::internal::(anonymous namespace)::ElementsKindTraits<(v8::internal::ElementsKind)2> >::GrowCapacity(v8::internal::Handle<v8::internal::JSObject>, unsigned int) [/Users/user/.nvm/versions/node/v10.16.0/bin/node]
 8: 0x1007a06bf v8::internal::Runtime_GrowArrayElements(int, v8::internal::Object**, v8::internal::Isolate*) [/Users/donghyun/.nvm/versions/node/v10.16.0/bin/node]
 9: 0x3557fc5be3d
10: 0x3558022e30f

Expected reason:

I tried to find the reason from the recent changes in PR(#50). I found some codes that expected bugs.

https://github.com/koajs/router/commit/e07397c43802c6c9b2064dd4994944b738cb3984#diff-504fd89f344be1e9c4b583f3ebd7428aL262

In this refactoring, there are three i are declared and effected themself. I think it occurs infinite loop.

https://github.com/koajs/router/commit/e07397c43802c6c9b2064dd4994944b738cb3984#diff-504fd89f344be1e9c4b583f3ebd7428aR283

Before refactoring, In this section(this[index] = cloneLayer) refered to index. But after refatoring it chaged like cloneRouter.stack[i] = cloneLayer.

I wonder where the index has been referenced and why I changed it to i.

cc. @JacobMGEvans

JacobMGEvans commented 4 years ago

The for loops i's are block-scoped to the for loop they are in so if the loops were nested then they would be affecting each other.

The index was the forEach index, when changing to an ES1 for loop it referenced the index of the for loop which is defined as i. The this was defined in the callback of the forEach at the end }, cloneRouter.stack) which sets the forEach local this to cloneRouter.stack

As for the heap memory issue, I wonder why the tests and manual checking didn't catch it. What was the code implementation like when you got this error? Maybe we can narrow down the culprit.

The only thing I think could be possibly causing some strange side effect is the function I madesetRouterParams() to create a closure for that particular for loop.

@niftylettuce any ideas?

Gusten commented 4 years ago

Our test suites are failing with "JavaScript heap out of memory" after upgrading to 8.0.7 as well, just like egaoneko is experiencing. Practically the same stacktrace as above. Only difference is that I'm using node 13.8.0.

egaoneko commented 4 years ago

@JacobMGEvans Oh, I miss watched about index with forEach. When I tested it, It does not occured with just koa and router. We use koa with Next.js, TypeScript, Webpack. I try to make a sample code for it.

JacobMGEvans commented 4 years ago

Our test suites are failing with "JavaScript heap out of memory" after upgrading to 8.0.7 as well, just like egaoneko is experiencing. Practically the same stacktrace as above. Only difference is that I'm using node 13.8.0.

@Gusten Would it be possible to see the tests that are failing from your code and unit it is testing?

Also what is the tech stack, @egaoneko mentioned that Next.js, TS and Webpack are being used in the project that this issue is occurring?

EDIT: Also, I am attempting to replicate the issue here https://codesandbox.io/s/koa-template-hv82k If anyone else can recreate the issue with this template please send me the link.

egaoneko commented 4 years ago

@JacobMGEvans I made a sample code causing infinite loops. https://codesandbox.io/s/koa-router-807-test-yj8c5

Router.prototype.use = function () {
  ...
  for (var i = 0; i < middleware.length; i++) {
    var m = middleware[i];
    if (m.router) {
      var cloneRouter = Object.assign(Object.create(Router.prototype), m.router, {
        stack: m.router.stack.slice(0)
      });

      for (var i = 0; i < cloneRouter.stack.length; i++) {
        var nestedLayer = cloneRouter.stack[i];
        var cloneLayer = Object.assign(
          Object.create(Layer.prototype),
          nestedLayer
        );

        if (path) cloneLayer.setPrefix(path);
        if (router.opts.prefix) cloneLayer.setPrefix(router.opts.prefix);
        router.stack.push(cloneLayer);
        cloneRouter.stack[i] = cloneLayer;
      }
  ...
  }

  return this;
};

This problem occurs in the above code when middlewares are bigger than nested layers. As I said before, this happens because of the nested i.

Unlike let andconst, var is a function scope, so this problem occurs. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let

Router.prototype.use = function () {
  ...
  for (var i = 0; i < middleware.length; i++) {
    var m = middleware[i];
    if (m.router) {
      var cloneRouter = Object.assign(Object.create(Router.prototype), m.router, {
        stack: m.router.stack.slice(0)
      });

      for (var i = 0; i < cloneRouter.stack.length; i++) {
        var nestedLayer = cloneRouter.stack[i];
        var cloneLayer = Object.assign(
          Object.create(Layer.prototype),
          nestedLayer
        );

        if (path) cloneLayer.setPrefix(path);
        if (router.opts.prefix) cloneLayer.setPrefix(router.opts.prefix);
        router.stack.push(cloneLayer);
        cloneRouter.stack[i] = cloneLayer;
      }
  ...
  }

  return this;
};

https://github.com/egaoneko/router/commit/8828d7b07ce9724d4ae16b7de3737c9af4a18df2#diff-504fd89f344be1e9c4b583f3ebd7428aL273

So I changed like the above code, it works.