Skip to content
This repository was archived by the owner on Nov 22, 2024. It is now read-only.

fix(platform-node): Remove memory leak #557

Merged
merged 1 commit into from
Sep 27, 2016
Merged

fix(platform-node): Remove memory leak #557

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions modules/platform-node/node-platform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ function s4() {
/*implements PlatformRef*/
export class NodePlatform {
static _noop = () => {};
static _cache = new Map<any, any>();
static _cache = new WeakMap<any, any>();
get platformRef() {
return this._platformRef;
}
Expand Down Expand Up @@ -332,7 +332,7 @@ export class NodePlatform {
let prebootCode = null;
// TODO(gdi2290): hide cache in (ngPreboot|UniversalPreboot)
let prebootConfig = null;
let key = (typeof preboot === 'object') && JSON.stringify(preboot) || null;
let key = (typeof preboot === 'object') && preboot || null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the goal of this cache was reusing the same prebootEl but I guess the biggest problem was probably not detaching it from the dom after destroying the app. If we move preboot to createEngine() which is the plan then this is fine since we're maintaining the same reference to the preboot config. During each req/res we're creating a new preboot config which was why I wanted to ensure if the configs are the same that we reuse the preboot element

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I was curious if that was why this was being stringified! Makes sense, and I frankly would've left it but WeakMaps don't support non-object keys so to get the memory releasing map it has to be the original object

Until moving to createEngine(), could also consider using an LRU or LFU cache in that case if you want to have serialized keys in this cache without risks of memory leaks. happy to send a followup PR if there is any other way you want this implemented here 👍

let prebootEl = null;
let el = null;
let lastRef = null;
Expand Down