Memory Leak in Tests

[jamesarosen]

I'm converting an Ember 1.13 app from ember-cli-ic-ajax to ember-ajax and my tests keep running out of memory. See the discussion below for the (likely) cause.

[jamesarosen]

Retainers:

container in Class
  __proto__ in Class
    service:ajax in @17562093
      cache in Container
        container in Class
          __proto__ in Class
            _this in system / Context
              context in function() (ajax-request.js:53)
                [1] in Array
                  [13] in Array
                    waiters in @16319955
                      Test in @16339013
                        Em in window

That line number is in ajax-request.init:

Test.registerWaiter(() => this.pendingRequestCount === 0);

The => keeps a closure reference to this (as _this). But I don't see a reference from the AjaxRequest to container anywhere. The reference is options.context = this, where this is the service:ajax, which clearly has a reference to the container.

[alexlafroscia]

Hey @jamesarosen! Thanks for reporting this. I've made a PR to remove the options.context = this bit that seems to be causing problems; it didn't seem to be necessary anyway. Would you mind giving that branch a try and letting me know if it fixes things for you?

[jamesarosen]

Thanks for the quick turnaround. Your proposed fix doesn't quite do it because even though options.context is no longer a pointer the service:ajax, _this is. There's no way to get around the fact that those waiters have a reference to the service:ajax. Could you maybe store pendingRequestCount on some other object that doesn't have a reference to service:ajax?

[jamesarosen]

You might be able to do it with a closure-local variable:

let pendingRequestCount = 0;

export default class AjaxRequest {
  // in case other clients of service:ajax need access to this variable:
  get pendingRequestCount() {
    return pendingRequestCount;
  }

  init() {
    pendingRequestCount = 0;
    if (testing) {
      Test.registerWaiter(function() { return pendingRequestCount === 0; });
    }
  }
}

Then the callback only has a handle on the module, which the requireJS global module system already has a handle on, so there's no new leak.

But that means the pending-request count is global, not per-instance of service:i18n. That's a behavior change that might be important, especially in test environments.

[alexlafroscia]

I don't think that would matter (having that variable being per-instance of service:ajax vs. global) but I'd like to ping @rwjblue on that to ask, since this implementation is based on his suggestion.

Also, thanks for the patience and good explanations; I've never dealt much with memory issues so this is a huge learning opportunity for me!

[alexlafroscia]

Also, just to I can understand why this was an issue, from a learning-more-about-JS perspective...

The issue is that Test.registerWaiter has a reference to an instance of the service, and isn't letting go of that after each test finishes. So, each test is generating at least one reference to the service that never is actually released, and it eventually becomes too much for the browser to handle.

Is that right?

[jamesarosen]

So, each test is generating at least one reference to the service that never is actually released, and it eventually becomes too much for the browser to handle.

Exactly! Though it's not the service:ajax that's the memory hog. It's the fact that the service is built from the container and thus has a reference to the container. That, in turn, has a reference to every route, controller, template, and service in the app. Our app has about 1000 acceptance tests. If we leak a container per test, we run into the many GB of memory usage.

[alexlafroscia]

Oh wow, I see. So, if it were just the ES6 class it wouldn't be (as much of) a problem, right?

[jamesarosen]

So, if it were just the ES6 class it wouldn't be (as much of) a problem

Probably not too bad, no. Still a leak, but most systems probably wouldn't notice.

[alexlafroscia]

Cool. If we used unregisterWaiter, do you think we avoid the issue of the container not being destroyed with each test? Maybe we could keep the per-instance counter, but still avoid the leak.

[jamesarosen]

If we used unregisterWaiter, do you think we avoid the issue of the container not being destroyed with each test?

It may. I don't know much about how Test.registerWaiter works. I'd be happy to try that variant as well.

[alexlafroscia]

@jamesarosen awesome, thanks. I pushed a commit that moves back to the per-instance count, and then tears down the waiter when the service is destroyed (which should be done at the end of each acceptance test).

[jamesarosen]

Now I see the leak again, though I can no longer tie it directly to this path, so I can't really say whether the unregisterWaiter method works.

[alexlafroscia]

Thanks for working with me on this @jamesarosen! I just released v2.3.2 that includes this fix.