From afbd563810180e65bb6815e66748a0517d1d0a4d Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Tue, 6 Mar 2018 11:33:56 +0000 Subject: [PATCH 1/3] Potentially fix a memory leak in FlairStore For each successful request of a group profile, we previously emitted an `updateGroupProfile` event per caller of `getGroupProfileCached`. This is sub-optimal because only a single event emit is required to update the views listening. It's possible that this was enabling some race to cause a memory leak but this is not certain, hence the extra logging for future debugging. --- src/stores/FlairStore.js | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/stores/FlairStore.js b/src/stores/FlairStore.js index 4ef29ae4e1..f8f76dd9af 100644 --- a/src/stores/FlairStore.js +++ b/src/stores/FlairStore.js @@ -154,16 +154,26 @@ class FlairStore extends EventEmitter { return this._groupProfiles[groupId]; } - // No request yet, start one - if (!this._groupProfilesPromise[groupId]) { - this._groupProfilesPromise[groupId] = matrixClient.getGroupProfile(groupId); + // A request is ongoing, wait for it to complete and return the group profile. + if (this._groupProfilesPromise[groupId]) { + try { + await this._groupProfilesPromise[groupId]; + } catch (e) { + // Don't log the error; this is done below + return undefined; + } + return this._groupProfiles[groupId]; } + // No request yet, start one + console.log('FlairStore: Request group profile of ' + groupId); + this._groupProfilesPromise[groupId] = matrixClient.getGroupProfile(groupId).delay(5000); + let profile; try { profile = await this._groupProfilesPromise[groupId]; } catch (e) { - console.log('Failed to get group profile for ' + groupId, e); + console.log('FlairStore: Failed to get group profile for ' + groupId, e); // Don't retry, but allow a retry when the profile is next requested delete this._groupProfilesPromise[groupId]; return; @@ -179,6 +189,7 @@ class FlairStore extends EventEmitter { /// XXX: This is verging on recreating a third "Flux"-looking Store. We really /// should replace FlairStore with a Flux store and some async actions. + console.log('FlairStore: Emit updateGroupProfile for ' + groupId); this.emit('updateGroupProfile'); setTimeout(() => { From 0fc79a4a0dc8746e763822b31bb4aa9dd617ccd1 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Tue, 6 Mar 2018 12:13:46 +0000 Subject: [PATCH 2/3] Remove delay used in testing --- src/stores/FlairStore.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/stores/FlairStore.js b/src/stores/FlairStore.js index f8f76dd9af..36b284664a 100644 --- a/src/stores/FlairStore.js +++ b/src/stores/FlairStore.js @@ -167,7 +167,7 @@ class FlairStore extends EventEmitter { // No request yet, start one console.log('FlairStore: Request group profile of ' + groupId); - this._groupProfilesPromise[groupId] = matrixClient.getGroupProfile(groupId).delay(5000); + this._groupProfilesPromise[groupId] = matrixClient.getGroupProfile(groupId); let profile; try { From faf517419ff13691928653e9d8a23dc07d1fbdbe Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Tue, 6 Mar 2018 12:14:41 +0000 Subject: [PATCH 3/3] Return null instead of undefined on failure --- src/stores/FlairStore.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/stores/FlairStore.js b/src/stores/FlairStore.js index 36b284664a..c8b4d75010 100644 --- a/src/stores/FlairStore.js +++ b/src/stores/FlairStore.js @@ -160,7 +160,7 @@ class FlairStore extends EventEmitter { await this._groupProfilesPromise[groupId]; } catch (e) { // Don't log the error; this is done below - return undefined; + return null; } return this._groupProfiles[groupId]; } @@ -176,7 +176,7 @@ class FlairStore extends EventEmitter { console.log('FlairStore: Failed to get group profile for ' + groupId, e); // Don't retry, but allow a retry when the profile is next requested delete this._groupProfilesPromise[groupId]; - return; + return null; } this._groupProfiles[groupId] = {