Non-view-related parent classes leak, even if DI lifetimes in the hierarchy are homogeneous

[estaylorco]

I'm submitting a bug report

• Library Version: 1.1.2 (aurelia-framework)

Please tell us about your environment:

• Operating System: OSX 10.12.4

• Node Version: 6.10.0

• NPM Version: 4.3.0

• Aurelia CLI OR JSPM OR Webpack AND Version JSPM 0.16.53

• Browser: all

• Language: ESNext

Current behavior: Even for non-view-related classes, inheriting from a base class (regardless of whether both parent and child are transient or singleton), causes the base class to leak.

Consider the following from my controller-authentication.js file:

@transient()
@inject(UsersGateway, SmsGateway)
export class AuthenticationController {
  constructor(usersGateway, smsGateway) {
    this._usersGateway = usersGateway;
    this._smsGateway = smsGateway;
  }
}

Now, if in the same file and before the above, I add the following:

@transient()
export class ControllerBase {
  constructor() {}
}

and then change the child class to extend from this parent class, everything is released from memory except the parent class. By everything I mean the following:

• Child class
• Any views-models where the child class is injected

Am I misunderstanding something? It seems to me that it shouldn't even be possible for just the parent class to hang around when it has the same DI lifetime. I know it's not a garden variety leak since both the view-models and the child classes are releasing.

Expected/desired behavior:

When a child class is released from memory, its parent class should also be released (up the hierarchy), so long as each class in the hierarchy has the same DI lifetime.

Also, I'm not advocating a support of inheritance in the context of custom elements, or any other DOM-related component. I'm simply referring to utility classes designed off to the side with all the richness of ES6.

• What is the motivation / use case for changing the behavior? To eliminate memory leaks.

[AshleyGrant]

Can you provide a more fleshed out example? As far as I'm aware, there isn't a separate instance of the parent class created when using inheritance, so I'm not following how the parent class isn't being released.

Also, even if there is a separate instance, the DI container isn't going to be aware of it, as the base class is "newed up" by a call to super in your constructor. Aurelia doesn't handle that.

[estaylorco]

Hi Ashley,

Actually, the code I put down in the post is actually more advanced than my test case. This was my test case:

@transient()
export class ControllerBase {
  constructor() {}
}

@transient()
export class AuthenticationController extends ControllerBase {
  constructor() {
      super();
  }
}

This simply example leaks, as written.

If Aurelia doesn't handle super, what is the proper way to do this? I'm pretty bummed out. I spent the day refactoring my controllers, gateways, and models to very simplistic inheritance (I happen to share the Team's opinion on composition vs. inheritance, with few exceptions), only to then discover I was leaking parent classes.

Thank you.

[AshleyGrant]

Can you provide a screenshot or something showing how this is leaking?

Also, I'm confused as to why you refactored to use inheritance if you're on board with us in the "favor composition over inheritance" bus. I would never recommend to anyone to use inheritance, simple or complex, in an Aurelia app, as inheritance and dependency injection go together like oil and water.

[estaylorco]

I have attached screenshots of the host class, which is the Login view-model class. This class does, in fact, release once the user logs in. No leaks there.

Into the Login class I import and inject the Child class, which inherits from Parent. As you can see from the memory profile, Child is released but Parent remains, even though Login has been released.

[AshleyGrant]

@EisenbergEffect is this a bug in Aurelia or is this an artifact of transpilation and thus a possible bug in Babel?

[estaylorco]

Now to your question about composition over inheritance. I just wanted to get the bug report out first...

I prefer composition over inheritance where composition is suitable. I came up through Smalltalk, Eiffel, C++, Java (now Kotlin for me), and C#, so it has been tough to shake inheritance. But it turns out that, in JavaScript, composition is fine most of the time anyways.

When I do have a proper is-a relationship, I expect inheritance to be there, whatever form that may take for a given language. And it is there in JavaScript. But I do not see inheritance->DI as a proper refactoring path. We can do it, of course. But we use Spring on the back end (I started with Spring 2.0 years ago), and I have found both inheritance and DI to work in harmony. I have never felt the need to refactor from inheritance to DI, nor do I have any peers who would even see that as a refactoring path.

Composition can create unnecessary complexity. We need to make sure that the classes we're composing are polymorphic (lot of repetition, even if minor); we need to delegate to inner instances, which means we have to guard against null; and in JavaScript we have no practical way of hiding those inner instances.

In any case, we're just two guys talking here. No complaints about Aurelia. Even in those instances where I feel inheritance would be more suitable, I'll use composition if that's what the Team advocates, and I'll promote that approach to others.

With that said, though, it seems that we do have a bug on our hands. My concern is that there is a larger problem underneath, which is why I thought I would bring it to your attention.

[estaylorco]

Ashley,

One additional point (which I'll create as a separate feature request if you think it has merit): What about an @unmanaged decorator?

When people start out with Spring, they want to make every class a bean: in other words, place every class under the management of DI. But not all classes should be beans. Would it be possible with Aurelia's design to provide a means of excluding classes from DI, let their instances simply go out of scope naturally? In Spring, we achieve this by simply leaving the class unmarked.

Just an idea...

[AshleyGrant]

But what I'm saying is that Aurelia isn't managing the lifecycle of the base class in the first place, you are. When you call super(), that is basically the same thing as new Parent(). The transient decorator on Parent is meaningless here.

As I mentioned above, I'm doubting this is a bug in Aurelia, and is more likely a bug in how Babel transpiles inheritance.

[AshleyGrant]

Also, I'm all for inheritance in entities in your application (since you new up those yourself, and don't get instances of them from the DI container), but for services and VMs and the such, I would avoid it at all costs in an Aurelia app. The Aurelia DI container doesn't support the property injection that would be necessary to fully support scenarios with inheritance, and I'd venture to guess it never will as we do not want to encourage the use of inheritance.

I'm not trying to argue about whether or not you've found something, because you obviously have, I'm just not sure if it's something we can fix in Aurelia, as I'm doubting Aurelia is what is keeping these parent "objects" around.

[estaylorco]

Yeah, you know, I think I need to just stick with composition, even in those cases where inheritance is probably the more suitable tool.

I wasn't trying to buck the trend or anything. My gateways, controllers, and models had taken sufficient form that I saw repetition that could be factored out. So I did it in a way that has made sense to me for years.

Thanks for your help!

[AshleyGrant]

In fact, I'm fairly sure this isn't an Aurelia bug. I just ran a test in a straight HTML file that looks like this:

<!DOCTYPE html>
<html>
    <body>
        <script>
        "use strict";

        var _createClass = function () { function defineProperties(target, props) { for (var i = 0; i < props.length; i++) { var descriptor = props[i]; descriptor.enumerable = descriptor.enumerable || false; descriptor.configurable = true; if ("value" in descriptor) descriptor.writable = true; Object.defineProperty(target, descriptor.key, descriptor); } } return function (Constructor, protoProps, staticProps) { if (protoProps) defineProperties(Constructor.prototype, protoProps); if (staticProps) defineProperties(Constructor, staticProps); return Constructor; }; }();

        function _possibleConstructorReturn(self, call) { if (!self) { throw new ReferenceError("this hasn't been initialised - super() hasn't been called"); } return call && (typeof call === "object" || typeof call === "function") ? call : self; }

        function _inherits(subClass, superClass) { if (typeof superClass !== "function" && superClass !== null) { throw new TypeError("Super expression must either be null or a function, not " + typeof superClass); } subClass.prototype = Object.create(superClass && superClass.prototype, { constructor: { value: subClass, enumerable: false, writable: true, configurable: true } }); if (superClass) Object.setPrototypeOf ? Object.setPrototypeOf(subClass, superClass) : subClass.__proto__ = superClass; }

        function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

        var Parent = function () {
            function Parent() {
                _classCallCheck(this, Parent);
            }

            _createClass(Parent, [{
                key: "doParentStuff",
                value: function doParentStuff() {
                    console.log("doing parent stuff");
                }
            }]);

            return Parent;
        }();

        var Child = function (_Parent) {
            _inherits(Child, _Parent);

            function Child() {
                _classCallCheck(this, Child);

                return _possibleConstructorReturn(this, (Child.__proto__ || Object.getPrototypeOf(Child)).call(this));
            }

            _createClass(Child, [{
                key: "doChildStuff",
                value: function doChildStuff() {
                    console.log("doing child stuff");
                }
            }]);

            return Child;
        }(Parent);

        function createIt() {
            var child = new Child();

            child.doParentStuff();
            child.doChildStuff();
        }

        createIt();

        console.log("Moving on");
    </script>
    </body>
</html>

The JavaScript is the output of Babel when I create the classes Child and Parent and have Child inherit from Parent. I just used the Babel playground on their website to get the code.

I ran this page using http-server and I see the following:

[EisenbergEffect]

@estaylorco Can you provide a simple repro that I can look at? From the debugger output, it looks like it's the child class in memory. There wouldn't be a parent class. So, I think something else is going on. There could be various reasons why the child class isn't GC'd, including looking at it in the debugger.

[estaylorco]

@AshleyGrant What's the conclusion, then? I take it that Babel is not involved in your sample?

[AshleyGrant]

@estaylorco I used babel to generate the JS code. That's the output from the playground on the Babel website (https://babeljs.io/repl). This is the code I used as the input:

class Parent {
  constructor() {

  }

  doParentStuff() {
    console.log("doing parent stuff");
  }
}

class Child extends Parent {
  constructor() {
    super();
  }

  doChildStuff() {
    console.log("doing child stuff");
  }
}

function createIt() {
  let child = new Child();

  child.doParentStuff();
  child.doChildStuff();
}

createIt();

console.log("Moving on");

I copied the output in to an HTML file and ran it.

[AshleyGrant]

Also, what Rob says makes sense, based on the heap dump I have. The instance is called Parent, but it has doChildStuff on it, which is defined in Child. Also, the constructor is called Child().

[estaylorco]

@EisenbergEffect Yes, I will do that. If I could have until tomorrow, that would be great. I've been coding for 12 of the last 14 hours :).

[AshleyGrant]

Yeah, I'm going to bed now too. Let me know what you figure out.

[EisenbergEffect]

There's no rush. Maybe get some rest and approach this fresh in the morning 😄

[estaylorco]

@AshleyGrant @EisenbergEffect Will do!

And, Ashley, it's quite a relief to know that you generated that code with Babel. I thought, man, if you're just banging that out on a whim, I should feel altogether inadequate :)

[AshleyGrant]

lol, if I could bang that code out on a whim, I'd be living in a bigger house, I think 👍

[estaylorco]

@EisenbergEffect @AshleyGrant O.K. guys. I created a public repository on my GitHub account that you can clone addressing this issue.

I started with the ESNext skeleton, created an inheritance.js file containing both Parent and Child classes, and then imported/injected Child into Users VM in users.js. I confirmed first that the skeleton's Users VM does, in fact, release from memory.

Sure enough, Parent leaks but Child does not. Please let me know if this is sufficient for you to test and explore.

Thank you!

[AshleyGrant]

I'm telling ya, @estaylorco, it isn't the Parent class that's leaking, it is the child class instance. You can see this if you add a method to both the parent and the child, like I did in my example code.

In fact, I just ran the native ES2015 code from my example in Chrome 58. It exhibits the same exact behavior. Even when I force Garbage Collection, the object sticks around. This is with no Aurelia to be found.

At this point, I'm 100% convinced this is not a bug in Aurelia. If anything, it's a bug in Chrome's JS engine, but I kind of doubt that.

[estaylorco]

@AshleyGrant Oh yes, you did say and show me that. When I see Parent but not Child, I rush to Parent as the culprit.

Last night, after I finished my exchange with you and @EisenbergEffect, I explored Babel's issues. I didn't see anything exotic or problematic with inheritance (other than with transpiling subclasses of built-in types, which seems to have been addressed).

This leads me to believe that it's probably not Babel, either. So if it's not Aurelia and it's not Babel, that would go to your theory that it's Chrome's JS engine. Maybe even the Chrome Dev Tools themselves.

@EisenbergEffect seemed to be keen last night on looking at this. Perhaps he can shed some light.

[AshleyGrant]

It has been fun playing with this, I have to say.

[estaylorco]

Agreed. Apart from the academic discourse on inheritance vs. composition, which I think has been resolved, I'm more curious now than ever.

Even if the party line is "composition over inheritance," I'm sure we want it to be for reasons other than some "weirdness" of inheritance that can't be explained.

[EisenbergEffect]

Aurelia holds no references to transient instances. In the referenced sample, the only object that holds the reference to Child is Users. If Users is collected, then Child should be as well. If it's not, then something outside of Aurelia is probably holding a reference. The prime suspect would be the debug tools.

@jdanyow Any further thoughts on this?

[jdanyow]

Any sort of console.log will cause a leak, I haven't read all the convo above ^^^, may not be relevant, but I wanted to call that out.