cds-icon
subscribes to state updates observable GlobalStateService.stateUpdates
in its firstUpdated
callback.
The unsubscribe logic for GlobalStateService.stateUpdates
is placed in the disconnectCallback
.
The memory leak happens because it's not guaranteed that disconnectCallback
will be called after firstUpdated
in terms of callback execution order. There are situations where disconnectCallback
is called first and then firstUpdated
is called. When this happens and disconnectCallback
is called first, the cds-icon
will 'leak' subscribers to the GlobalStateService.stateUpdates
observable. These leaked subscribers are never unsubscribed and this leads to the memory leak.
The reason why disconnectCallback
is sometimes called before firstUpdated
is related to the way LitElement.requestUpdated()
works - this is the code https://github.com/lit/lit/blob/93d671feab82688a79fc60ba22cf204fa4ca02ec/packages/reactive-element/src/reactive-element.ts#L1150.
In short, requestUpdated()
works asynchronously which means the firstUpdated
callback will be called asynchronously. Since disconnectCallback
works synchronously there are situations where firstUpdated
will be called after disconnectCallback
.
How to reproduce
To reproduce the problem, you can run unit test spec file using Angular 13 with testing module teardown.destroyAfterEach
enabled (teardown is enabled by default in Angular 13).
In the spec below, every it
will leak 1 subscriber to the GlobalStateService.stateUpdates
, that's why the expectations (expect
) in the consecutive it(...)
are for 1, 2, 3 and 4 subscribers respectively.
NOTE: if you run the spec file, make sure the spec randomized execution is disabled in your karma.conf.js
. Also, you must use Angular 13 with teardown enabled. By doing this, the disconnectCallback
will be called before firstUpdated
consistently. In Angular 13 projects (with teardown enabled) that have a lot of unit tests, these leaked subscribers cause significant test slowdown as the test execution progresses.
SPEC FILE CODE
import { Component, CUSTOM_ELEMENTS_SCHEMA } from '@angular/core';
import { TestBed } from '@angular/core/testing';
import { ClarityIcons, popOutIcon } from '@cds/core/icon';
import { CdsModule } from '@cds/angular';
import { GlobalStateService } from '@cds/core/internal';
ClarityIcons.addIcons(popOutIcon);
@Component({
selector: 'test-component',
template: `
<cds-icon shape="pop-out"></cds-icon>
`
})
export class TestComponent {
}
describe('Test GlobalStateService.stateUpdates.subscriptions Memory Leak', () => {
beforeEach(() => {
TestBed.configureTestingModule({
imports: [
CdsModule
],
declarations: [
TestComponent
],
schemas: [
CUSTOM_ELEMENTS_SCHEMA
],
teardown: {
// With Angular 13 'destroyAfterEach' is enabled by default.
destroyAfterEach: true
}
})
});
it('create icon for the first time - currently there are no subscribers in the global service', () => {
expect(((GlobalStateService.stateUpdates as any).subscriptions as any[]).length).toEqual(0);
const fixture = TestBed.createComponent(TestComponent);
fixture.detectChanges();
// At this point the 'cds-icon' element is added to the DOM and element's connectedCallback() has been called.
// But there are still no subscribers in the global service because CdsIcon.firstUpdated() callback is not called yet.
expect(((GlobalStateService.stateUpdates as any).subscriptions as any[]).length).toEqual(0);
});
it('verify 1 subscriber is currently leaked in the global service', () => {
expect(((GlobalStateService.stateUpdates as any).subscriptions as any[]).length).toEqual(1);
const fixture = TestBed.createComponent(TestComponent);
fixture.detectChanges();
expect(((GlobalStateService.stateUpdates as any).subscriptions as any[]).length).toEqual(1);
});
it('verify 2 subscribers are currently leaked in the global service', () => {
expect(((GlobalStateService.stateUpdates as any).subscriptions as any[]).length).toEqual(2);
const fixture = TestBed.createComponent(TestComponent);
fixture.detectChanges();
expect(((GlobalStateService.stateUpdates as any).subscriptions as any[]).length).toEqual(2);
});
it('verify 3 subscribers are currently leaked in the global service', () => {
expect(((GlobalStateService.stateUpdates as any).subscriptions as any[]).length).toEqual(3);
const fixture = TestBed.createComponent(TestComponent);
fixture.detectChanges();
expect(((GlobalStateService.stateUpdates as any).subscriptions as any[]).length).toEqual(3);
});
});
How to reproduce in production code
The easiest way to reproduce the issue is with the following code:
const icon = document.createElement('cds-icon');
icon.setAttribute('shape', 'pop-out');
document.body.appendChild(icon);
icon.remove();
Creating an icon element, attaching it to the DOM and then immediately removing it will result in leaked subscribers.
Here is a stackblitz that uses the code above to leak 1000 subscriptions https://stackblitz.com/edit/clarity-dark-theme-clr13-cds6-duukxq?file=src/app/app.component.html
Expected behavior
Adding and removing cds-icon from the DOM should not leak subscribers to GlobalStateService.stateUpdates
.
It seems firstUpdated()
callback is not the place where cds-icon (and possibly other components from Clarity Core) should subscribe to GlobalStateService.stateUpdates
.
Probably, the connectedCallback()
is the place where the subscribe logic should be placed.
Versions
Clarity project:
Clarity version:
Framework:
Framework version:
Angular 13
Device:
- Type: MacBook
- OS: MacOS Monterey
- Browser: Chrome
- Version: 100.0.4896.127