cds-icon causes memory leak and slow Angular v13 unit tests

[petarbk]

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 Core
• Clarity Angular/UI

Clarity version:

• v3.x
• v4.x
• v5.x

Framework:

• Angular
• React
• Vue
• Other:

Framework version:
Angular 13

Device:

• Type: MacBook
• OS: MacOS Monterey
• Browser: Chrome
• Version: 100.0.4896.127

[github-actions]

🎉 This issue has been resolved in version 5.7.3 🎉

The release is available on:

• GitHub release
• v5.7.3

Your semantic-release bot 📦🚀

[github-actions]

🎉 This issue has been resolved in version 6.0.1 🎉

The release is available on:

• GitHub release
• v6.0.1

Your semantic-release bot 📦🚀

[github-actions]

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed issues after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.