Service's willDestroy hook is not triggered if it was initialised in another service's willDestroy

[andreyfel]

The issue is mostly related to application tests because services are usually not destroyed while the app is running. However, it can be a reason of a significant memory leak in tests.

Reproduction repo:
https://github.com/andreyfel/ember-services-teardown-bug

In ServiceA's willDestroy hook we call hi() method of the ServiceB. Until that moment ServiceB hasn't been invoked in any way, so, that call triggered init() of the ServiceB. ServiceB's willDestroy hook is not triggered. In order to see the issue run tests and look at the console.

Expected output:

ServiceA init
Hi from ServiceA
ServiceA willDestroy
ServiceB init
Hi from ServiceB
ServiceB willDestroy <---- It never shows up in the console.

[rwjblue]

The basic cleanup flow is:

• loop through all the singleton instances that have been created, and run.schedule('destroy', instance, 'willDestroy')
• later, while the destroy queue is flushing each of the previously scheduled functions are invoked
• ServiceA is instantiating ServiceB (by looking it up and calling a method)

Since ServiceA is instantiating ServiceB during teardown, and we never attempt to loop through instances a second time, we never realize that ServiceB has never been destroyed.

In general this shouldn't be a major issue (though I agree its pretty bizarre and we should try if we can to fix it), because ServiceB should eventually be garbage collected anyways (though this may of course be thwarted by something ServiceB is actually doing).

AFAICT, the only thing we can actually do to help here would be in the test helpers themselves. Essentially, after all queues have flushed we can confirm that all singletons have been destroyed (if not, loop through them again and destroy them). This could result in some sort of recursive loop, but I think that seems unlikely.

[andreyfel]

It is even fine if we just fail the test if a singleton hasn't been destroyed after all queues have flushed because this is kind of invalid behavior and probably there is a bug in the code. So, no need for the second attempt.

Another option is to throw an exception if some code tries to initialize a new singleton during the destroy queue flushing. It will expose the bug even earlier.

[rwjblue]

Another option is to throw an exception if some code tries to initialize a new singleton during the destroy queue flushing.

Agreed. This seems like the best option, essentially throw on any new instantiation done from the owner/container IFF the owner/container are in the isDestroying state.

Possible fix is to change this assertion to check this.isDestroying instead of this.isDestroyed:

ember.js/packages/@ember/-internals/container/lib/container.ts

Line 152 in 0d1cc3f

assert('expected container not to be destroyed', !this.isDestroyed);

[rwjblue]

This was fixed in #18717 and released in 3.16.2.