Internal: Solve some memory leak issue in app and tests (#1029)

timotheeguerin · web-flow · commit edc4068900e9 · 2018-01-29T15:40:01.000-07:00
diff --git a/app/components/application/browse/application-list.component.ts b/app/components/application/browse/application-list.component.ts
@@ -67,6 +67,7 @@ export class ApplicationListComponent extends ListOrTableBase implements OnInit,
 
     public ngOnDestroy() {
         this._subs.forEach(x => x.unsubscribe());
+        this.data.dispose();
     }
 
     @autobind()
diff --git a/app/components/application/details/application-details.component.ts b/app/components/application/details/application-details.component.ts
@@ -66,6 +66,7 @@ export class ApplicationDetailsComponent implements OnInit, OnDestroy {
 
     public ngOnDestroy() {
         this._paramsSubscriber.unsubscribe();
+        this.data.dispose();
     }
 
     @autobind()
diff --git a/app/components/job/action/add/pool-picker.component.ts b/app/components/job/action/add/pool-picker.component.ts
@@ -60,6 +60,7 @@ export class PoolPickerComponent implements ControlValueAccessor, OnInit, OnDest
 
     public ngOnDestroy() {
         this._subs.forEach(x => x.unsubscribe());
+        this.poolsData.dispose();
     }
 
     public writeValue(poolInfo: any) {
diff --git a/app/components/job/details/job-progress-status/job-progress-status.component.spec.ts b/app/components/job/details/job-progress-status/job-progress-status.component.spec.ts
@@ -3,13 +3,13 @@ import { ComponentFixture, TestBed } from "@angular/core/testing";
 import { By } from "@angular/platform-browser";
 import { Observable } from "rxjs";
 
-import { JobProgressStatusComponent } from "app/components/job/details/job-progress-status";
 import { Job, JobTaskCounts, Node, Pool } from "app/models";
 import { JobService, NodeService, PoolService } from "app/services";
 import { PollService } from "app/services/core";
 import { click } from "test/utils/helpers";
 import { MockEntityView, MockListView } from "test/utils/mocks";
 import { GaugeMockComponent } from "test/utils/mocks/components";
+import { JobProgressStatusComponent } from "./job-progress-status.component";
 
 @Component({
     template: `<bl-job-progress-status [job]="job" [poolId]="poolId"></bl-job-progress-status>`,
@@ -59,7 +59,7 @@ describe("JobProgressStatusComponent", () => {
         };
 
         pollServiceSpy = {
-            startPoll: () => ({}),
+            startPoll: () => ({destroy: () => null}),
         };
 
         TestBed.configureTestingModule({
diff --git a/app/components/job/details/job-progress-status/job-progress-status.component.ts b/app/components/job/details/job-progress-status/job-progress-status.component.ts
@@ -1,6 +1,5 @@
 import { Component, Input, OnChanges, OnDestroy } from "@angular/core";
 import { List } from "immutable";
-import { Subscription } from "rxjs";
 
 import { GaugeConfig } from "app/components/base/graphs/gauge";
 import { Job, JobTaskCounts, Node, Pool } from "app/models";
@@ -15,11 +14,9 @@ import "./job-progress-status.scss";
     templateUrl: "job-progress-status.html",
 })
 export class JobProgressStatusComponent implements OnChanges, OnDestroy {
-    @Input()
-    public job: Job;
+    @Input() public job: Job;
 
-    @Input()
-    public poolId: string;
+    @Input() public poolId: string;
 
     public nodes: List<Node> = List([]);
     public pool: Pool;
@@ -38,7 +35,6 @@ export class JobProgressStatusComponent implements OnChanges, OnDestroy {
     private poolData: EntityView<Pool, PoolParams>;
 
     private _polls: PollObservable[] = [];
-    private _subs: Subscription[] = [];
 
     constructor(
         poolService: PoolService,
@@ -54,20 +50,20 @@ export class JobProgressStatusComponent implements OnChanges, OnDestroy {
 
         this.updateGaugeOptions();
 
-        this._subs.push(this.poolData.item.subscribe((pool) => {
+        this.poolData.item.subscribe((pool) => {
             this.pool = pool;
             this.maxRunningTasks = pool ? pool.targetNodes * pool.maxTasksPerNode : 1;
             this.updateGaugeOptions();
-        }));
+        });
 
-        this._subs.push(this.data.items.subscribe((nodes) => {
+        this.data.items.subscribe((nodes) => {
             if (this.nodes.size !== nodes.size) {
                 this.poolData.refresh();
             }
             this.nodes = nodes;
             this.countRunningTasks();
             this.updateGaugeOptions();
-        }));
+        });
 
         this._polls.push(this.data.startPoll(refreshRate));
 
@@ -89,7 +85,7 @@ export class JobProgressStatusComponent implements OnChanges, OnDestroy {
 
     public ngOnDestroy() {
         this._polls.forEach(x => x.destroy());
-        this._subs.forEach(x => x.unsubscribe());
+        this.data.dispose();
         this.poolData.dispose();
     }
 
diff --git a/app/components/node/connect/node-connect.component.ts b/app/components/node/connect/node-connect.component.ts
@@ -57,9 +57,10 @@ export class NodeConnectComponent implements OnInit {
     public ngOnInit() {
         const data = this.nodeService.listNodeAgentSkus();
         data.fetchAll().subscribe(() => {
-            data.items.first().subscribe((agentSkus) => {
+            data.items.take(1).subscribe((agentSkus) => {
                 this.agentSkus = agentSkus;
                 this.windows = PoolUtils.isWindows(this.pool, agentSkus);
+                data.dispose();
             });
         });
         this._loadConnectionData();
diff --git a/app/components/pool/action/add/app-license-picker.component.ts b/app/components/pool/action/add/app-license-picker.component.ts
@@ -1,4 +1,4 @@
-import { Component, OnInit, ViewChild, ViewContainerRef, forwardRef } from "@angular/core";
+import { Component, OnInit, ViewChild, forwardRef } from "@angular/core";
 import {
     ControlValueAccessor, FormControl, NG_VALIDATORS, NG_VALUE_ACCESSOR, Validator,
 } from "@angular/forms";
@@ -37,7 +37,6 @@ export class AppLicensePickerComponent implements ControlValueAccessor, OnInit,
     private _eulaRead: boolean = false;
 
     constructor(
-        private viewContainerRef: ViewContainerRef,
         private dialog: MatDialog) {
     }
 
@@ -103,7 +102,6 @@ export class AppLicensePickerComponent implements ControlValueAccessor, OnInit,
 
     public viewEula(license: ApplicationLicense) {
         const config = new MatDialogConfig();
-        config.viewContainerRef = this.viewContainerRef;
         const dialogRef = this.dialog.open(LicenseEulaDialogComponent, config);
         dialogRef.componentInstance.license = license;
     }
diff --git a/app/components/pool/base/app-packages/app-package-picker.component.ts b/app/components/pool/base/app-packages/app-package-picker.component.ts
@@ -68,7 +68,7 @@ export class AppPackagePickerComponent implements ControlValueAccessor, Validato
         };
 
         // subscribe to the application data proxy
-        this._subscriptions.push(this._data.items.subscribe((applications) => {
+        this._data.items.subscribe((applications) => {
             this._applicationMap = {};
             if (applications.size > 0) {
                 this._mapApplicationPackages(applications);
@@ -83,7 +83,7 @@ export class AppPackagePickerComponent implements ControlValueAccessor, Validato
                     index++;
                 });
             }
-        }));
+        });
 
         // subscribe to the form change events
         this._subscriptions.push(this.items.valueChanges.subscribe((references: PackageReference[]) => {
@@ -114,6 +114,7 @@ export class AppPackagePickerComponent implements ControlValueAccessor, Validato
     }
 
     public ngOnDestroy() {
+        this._data.dispose();
         this._subscriptions.forEach(x => x.unsubscribe());
     }
 
diff --git a/app/services/core/data-cache.ts b/app/services/core/data-cache.ts
@@ -23,6 +23,13 @@ export class DataCacheTracker {
         }
     }
 
+    public static disposeAll() {
+        for (const cache of ObjectUtils.values(this._caches)) {
+            cache.clear();
+            this.unregisterCache(cache);
+        }
+    }
+
     private static _caches: { [key: string]: DataCache<any> } = {};
 }
 
@@ -70,7 +77,6 @@ export class DataCache<T> {
     public dispose() {
         this.clear();
         DataCacheTracker.unregisterCache(this);
-        // TODO implement dispose
     }
 
     /**
diff --git a/app/services/core/data/generic-view.ts b/app/services/core/data/generic-view.ts
@@ -90,6 +90,11 @@ export abstract class GenericView<TEntity, TParams, TOptions extends ProxyOption
         });
 
         this.deleted = this._deleted.asObservable();
+        this.init();
+    }
+
+    public init() {
+        // Nothing to do. Used in test for stub;
     }
 
     public set params(params: TParams) {
diff --git a/app/services/core/poll-service.ts b/app/services/core/poll-service.ts
@@ -73,6 +73,7 @@ export class PollService {
         if (!tracker) {
             return;
         }
+
         delete this._pollTrackers[key][id];
         if (tracker.running) {
             this._clearActivePoll(key);
diff --git a/app/utils/component-utils/component-utils.spec.ts b/app/utils/component-utils/component-utils.spec.ts
@@ -73,6 +73,10 @@ describe("ComponentUtils", () => {
             view.items.subscribe(x => items = x);
         });
 
+        afterEach(() => {
+            view.dispose();
+        });
+
         it("prepend the missing item to the begining of the list", () => {
             childRoute = {
                 params: { id: "pool-3" },
diff --git a/config/karma.shim.js b/config/karma.shim.js
diff --git a/karma.conf.js b/karma.conf.js
@@ -9,7 +9,7 @@ module.exports = function(config) {
         basePath: ".",
         frameworks: ["jasmine"],
         files: [{
-            pattern: "./config/karma.shim.js",
+            pattern: "./test/app/spec-entry.js",
             watched: false
         }, ],
 
@@ -46,7 +46,7 @@ module.exports = function(config) {
         reporters: ["mocha", ...coverageReporters],
 
         preprocessors: {
-            "config/karma.shim.js": ["coverage", "webpack", "sourcemap", "electron"],
+            "test/app/spec-entry.js": ["coverage", "webpack", "sourcemap", "electron"],
         },
         client: {
             useIframe: false,
diff --git a/test/app/services/core/data/entity-view.spec.ts b/test/app/services/core/data/entity-view.spec.ts
@@ -126,6 +126,7 @@ describe("EntityView", () => {
                 supplyData: dataSpy,
             });
 
+            view.dispose();
             view = new EntityView({
                 cache: () => cache,
                 getter: getter,
@@ -138,7 +139,7 @@ describe("EntityView", () => {
                 cache: () => cache,
                 supplyData: () => Observable.throw(new ServerError({ status: 404, message: "404 not found" } as any)),
             });
-
+            view.dispose();
             view = new EntityView({
                 cache: () => cache,
                 getter: getter,
@@ -172,6 +173,7 @@ describe("EntityView", () => {
             tick();
 
             expect(deleted).toEqual("1");
+            otherView.dispose();
         }));
 
         it("should not send to deleted if loading a new item that doesn't exist", fakeAsync(() => {
diff --git a/test/app/spec-controls.ts b/test/app/spec-controls.ts
diff --git a/test/app/spec-entry.js b/test/app/spec-entry.js
diff --git a/test/app/spec-imports.ts b/test/app/spec-imports.ts
diff --git a/test/app/spec-reporters.ts b/test/app/spec-reporters.ts
diff --git a/test/app/spec-runner.ts b/test/app/spec-runner.ts

Original file line number	Diff line number	Diff line change
`@@ -67,6 +67,7 @@ export class ApplicationListComponent extends ListOrTableBase implements OnInit,`
`67`	`67`
`68`	`68`	`public ngOnDestroy() {`
`69`	`69`	`this._subs.forEach(x => x.unsubscribe());`
	`70`	`+ this.data.dispose();`
`70`	`71`	`}`
`71`	`72`
`72`	`73`	`@autobind()`
Original file line number	Diff line number	Diff line change
`@@ -66,6 +66,7 @@ export class ApplicationDetailsComponent implements OnInit, OnDestroy {`
`66`	`66`
`67`	`67`	`public ngOnDestroy() {`
`68`	`68`	`this._paramsSubscriber.unsubscribe();`
	`69`	`+ this.data.dispose();`
`69`	`70`	`}`
`70`	`71`
`71`	`72`	`@autobind()`
Original file line number	Diff line number	Diff line change
`@@ -60,6 +60,7 @@ export class PoolPickerComponent implements ControlValueAccessor, OnInit, OnDest`
`60`	`60`
`61`	`61`	`public ngOnDestroy() {`
`62`	`62`	`this._subs.forEach(x => x.unsubscribe());`
	`63`	`+ this.poolsData.dispose();`
`63`	`64`	`}`
`64`	`65`
`65`	`66`	`public writeValue(poolInfo: any) {`
Original file line number	Diff line number	Diff line change
`@@ -23,6 +23,13 @@ export class DataCacheTracker {`
`23`	`23`	`}`
`24`	`24`	`}`
`25`	`25`
	`26`	`+ public static disposeAll() {`
	`27`	`+ for (const cache of ObjectUtils.values(this._caches)) {`
	`28`	`+ cache.clear();`
	`29`	`+ this.unregisterCache(cache);`
	`30`	`+ }`
	`31`	`+ }`
	`32`	`+`
`26`	`33`	`private static _caches: { [key: string]: DataCache<any> } = {};`
`27`	`34`	`}`
`28`	`35`
`@@ -70,7 +77,6 @@ export class DataCache<T> {`
`70`	`77`	`public dispose() {`
`71`	`78`	`this.clear();`
`72`	`79`	`DataCacheTracker.unregisterCache(this);`
`73`		`- // TODO implement dispose`
`74`	`80`	`}`
`75`	`81`
`76`	`82`	`/**`