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

Declarations do not get cleaned up unless you set them to null #1991

Closed
rodoabad opened this issue Dec 2, 2015 · 19 comments
Closed

Declarations do not get cleaned up unless you set them to null #1991

rodoabad opened this issue Dec 2, 2015 · 19 comments
Labels
status: accepting prs Mocha can use your help with this one!

Comments

@rodoabad
Copy link

rodoabad commented Dec 2, 2015

@boneskull Just wondering if we should be actively cleaning up out declarations per describe block.

Leaking

Mocha Memory Leak

(function () {

  'use strict';

  function MemoryLeak() {
  }

  for (var i = 0; i < 1000; i += 1) {

    describe('Suite #' + i, function () {

      var suite;

      beforeEach(function () {

        suite = new MemoryLeak();

      });

      it('should do something', function () {

        suite.isLeaking = true;

      });

    });

  }

})();

Not Leaking

Mocha Memory Leak

(function () {

  'use strict';

  function MemoryLeak() {
  }

  for (var i = 0; i < 1000; i += 1) {

    describe('Suite #' + i, function () {

      var suite;

      beforeEach(function () {

        suite = new MemoryLeak();

      });

      afterEach(function () {

        suite = null;

      });

      it('should do something', function () {

        suite.isLeaking = true;

      });

    });

  }

})();

More here - https://github.com/rodoabad/mocha-memory-leak

@boneskull
Copy link
Contributor

how do we know we're not just waiting on the garbage collector?

@rodoabad
Copy link
Author

rodoabad commented Dec 3, 2015

Shouldn't gc run after each test?

@tinganho
Copy link
Contributor

tinganho commented Dec 5, 2015

I have also experienced this. Mocha is storing the callbacks and it won't clean up because the runtime doesn't know if you will call those "callbacks" or not. And the memory footprint can be really substantial if your test suite is large. So I agree that it should clean up after each test.

@boneskull boneskull added the status: accepting prs Mocha can use your help with this one! label Dec 5, 2015
@boneskull
Copy link
Contributor

feel free to try and fix this; I'm probably going to be heads-down on a rewrite for awhile.

@gustiando
Copy link

Anyone know a solution to prevent this other than manually cleaning up after each test? I created a related issue here #2030 thanks!

@danielstjules
Copy link
Contributor

Shouldn't gc run after each test?

Nope, just like your node server won't run gc between each request. Here's an example:

// node --trace-gc --max-old-space-size=10 testing.js

var fn;
var obj;
var i = 0;

setInterval(function() {
  fn = function() {
    // Create some objects
    obj = [];
    for (var j = 0; j < 100; j++) {
      obj.push(j * j);
    }

    if (++i % 1000 === 0) {
      console.log(i);
    }
  };

  fn();
}, 1);

You'll likely only see Scavenge (which collects objects in new space) get invoked every 1000 invocations of fn, which includes 1000 allocations of those medium sized arrays, as well as a new fn being assigned to the outer scope. You probably shouldn't see the MarkSweep collector during that test. If interested, here's some more info: http://jayconrod.com/posts/55/a-tour-of-v8-garbage-collection

Mocha is storing the callbacks and it won't clean up because the runtime doesn't know if you will call those "callbacks" or not.

If a single test invokes the "done" callback once at the start of the suite, and a second time right before the test suite ends, wouldn't you want to know about this error? You can only do that by keeping reference to old test objects, though we could certainly delete the functions from test runnables (not hooks) after they've been ran. Might help a bit.

@danielstjules
Copy link
Contributor

@rodoabad I think your test is too small. If you increased the number of iterations, you should start seeing GC kick in.

@bd82
Copy link
Contributor

bd82 commented Dec 31, 2015

Hello.

I can confirm I've encountered this as well.

I've upgraded @rodoabad reproduction to:

  • both use more memory.
  • included a second scenario involving a workaround.

The cause is that mocha keeps references to test functions (it/before/after/...) to allow
deferred execution. if these functions have closures with 'external' variables those variables
will never be GCed because they are always referenced.

The hack workaround is to manually cleanup mocha references on "suite end" event.

    runner.on('suite end', function (suite) {
        delete suite.tests;
        delete suite._beforeAll;
        delete suite._beforeEach;
        delete suite._afterEach;
        delete suite.ctx;
        delete suite._afterAll;
    });

this cleans up too much and probably needs to be more precise...
but the fact it solves the problem helps prove the root cause.

@boneskull: how do we know we're not just waiting on the garbage collector?

Taking a heap snapshot forces garbage collection beforehand

Are "dead" (unreachable) objects included in snapshots?
No. Only reachable objects are included in snapshots. Also, taking a snapshot always starts with doing a GC.

<

@gumatias: Anyone know a solution to prevent this other than manually cleaning up after each test? I created a related issue here #2030 thanks!

Try The workaround above. does it solve your problem?

@gustiando
Copy link

@bd82 thank you very much for the comment!

I tried your suggestion, however I didn't see much difference (unless I'm doing it wrong). Since I'm using karma-mocha I had to stick my suite end listener somewhere else:

Some profiling results:

With clean up in suite end event:

Without clean up in runner suite end

@bd82
Copy link
Contributor

bd82 commented Jan 2, 2016

@gumatias Thanks for trying the hack.

Some thoughts:

  1. The time-line memory usage in the example looks similar with or without the hack too. it is the heap snapshot that looks differently as it runs GC first
    and shows the real' memory usage. can you try and check the result of a heap snapshot before/after.
  2. Which version of mocha are you using? the hack depends on internal mocha data structures
    so there may be a difference. I am asking because apparently you are using chrome 39 from 09/2014 and a karma version from 06/2015.
  3. The memory leak in your use case may not originate from mocha.
  4. The hack may not delete all the references.
    • I will try to make a proper PR next week with hopefully a more precise version.
  5. How are your specs written? do they use some module implementation(require.js/js module pattern)?
    if not you may be inadvertently creating globally scoped variables if you define vars outside
    the describe functions. those variables will never be GCed as they are referenced by the window object.

we also use karma-mocha in the production code that suffered from these issues.
(we have had to split up test suites of three different teams due to timeouts by memory leaks)

Additionally we use require.js so we have a test-main.js file that configures require.js and starts the tests.
I've added a different hack there to avoid modifying mocha.js directly.

in test-main.js

    var orgMochaRun = mocha.run;

    mocha.run = function(fn) {
        var runner = orgMochaRun(fn);

        runner.on('suite end', function(suite) {
            delete suite.tests;
            delete suite._beforeAll;
            delete suite._beforeEach;
            delete suite._afterEach;
            delete suite.ctx;
            delete suite._afterAll;
        });
    };

@gustiando
Copy link

  1. @bd82 absolutely! Below is a snapshot comparison of before and after when using delete on suite end. If I'm seeing it right, doesn't look like a lot have been freed.

  1. "mocha": "^2.3.4". My local chrome is indeed old, but on my CI server it's 43.x.x.x, which gets the timeout too so that might not be an issue.
  2. Sounds good! let me know if you need any help :)
  3. Forgive my "newbiness" in the subject, but I believe the project I'm working on does have the tests on some module implementation as its karma config file starts with:
module.exports = function(config) {
  ...
};

Our tests are also split into multiple executions. However, splitting our tests into even smaller parts will just keep hiding the root cause of the problem.

Thanks!

@bd82
Copy link
Contributor

bd82 commented Jan 3, 2016

I've suggested a pull request.
It includes a test that reproduces the issue (without the fix).

@bd82
Copy link
Contributor

bd82 commented Jan 3, 2016

@gumatias

I'm not sure your problem is that exactly the one I am trying to solve in the pull request.
You can try to include the changes in #2037.

If you do try it, you should take a heap snapshot and compare heap size.

comparing before / after proves you have a memory leak, but does not show the effects
of the code changes.

if you can upload one of your spec files I can have a look and see if you are using global
variables which will never be GCed...

@PixnBits
Copy link

FWIW @rodoabad @danielstjules It appears that PhantomJS runs garbage collection only on page close

@rcollette
Copy link

There would seem to be more to the story than just page close GC. If GC wasn't run at all until page close, I would think that nulling local vars would have no impact, unless there are separate types/phases of GC where nulling a local var immediately triggers GC. I'm more inclined to believe this has to do function references being held onto.

@stale
Copy link

stale bot commented Oct 17, 2017

I am a bot that watches issues for inactivity.
This issue hasn't had any recent activity, and I'm labeling it stale. In 14 days, if there are no further comments or activity, I will close this issue.
Thanks for contributing to Mocha!

@stale stale bot added the stale this has been inactive for a while... label Oct 17, 2017
@boneskull boneskull removed the stale this has been inactive for a while... label Oct 17, 2017
@jjoekoullas
Copy link

Apologies for necromancy, but this issue is still open and I do not think it is a problem anymore.

See rodoabad/mocha-memory-leak#1 - I was not able to reproduce this issue using the latest version of mocha. I've asked @rodoabad to take a peek and consider closing this issue if he agrees.

@boneskull
Copy link
Contributor

@jjoekoullas thanks.

@rodoabad
Copy link
Author

rodoabad commented Jun 6, 2019

@boneskull @jjoekoullas I'll be closing this issue now.

To everyone else, if you think this is still an issue, feel free to reference this issue.

@rodoabad rodoabad closed this as completed Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: accepting prs Mocha can use your help with this one!
Projects
None yet
Development

No branches or pull requests

9 participants