From 5f3c06b38a9ab96716b765394c4f04513f734aff Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 19 Oct 2017 10:28:59 +0100 Subject: [PATCH 1/3] Factor out Flair cache into FlairStore This will make invalidating the userGroups cache for the user architecturally more sound (the plan is to have GroupStore hit FlairStore as opposed to Flair itself in order to invalidate the cache). --- src/components/views/elements/Flair.js | 137 +--------------------- src/stores/FlairStore.js | 156 +++++++++++++++++++++++++ 2 files changed, 162 insertions(+), 131 deletions(-) create mode 100644 src/stores/FlairStore.js diff --git a/src/components/views/elements/Flair.js b/src/components/views/elements/Flair.js index ceab20bf19..f0bfd817dd 100644 --- a/src/components/views/elements/Flair.js +++ b/src/components/views/elements/Flair.js @@ -20,123 +20,9 @@ import React from 'react'; import PropTypes from 'prop-types'; import {MatrixClient} from 'matrix-js-sdk'; import UserSettingsStore from '../../../UserSettingsStore'; +import FlairStore from '../../../stores/FlairStore'; import dis from '../../../dispatcher'; -import Promise from 'bluebird'; -const BULK_REQUEST_DEBOUNCE_MS = 200; - -// Does the server support groups? Assume yes until we receive M_UNRECOGNIZED. -// If true, flair can function and we should keep sending requests for groups and avatars. -let groupSupport = true; - -const USER_GROUPS_CACHE_BUST_MS = 1800000; // 30 mins -const GROUP_PROFILES_CACHE_BUST_MS = 1800000; // 30 mins - -// TODO: Cache-busting based on time. (The server won't inform us of membership changes.) -// This applies to userGroups and groupProfiles. We can provide a slightly better UX by -// cache-busting when the current user joins/leaves a group. -const userGroups = { - // $userId: ['+group1:domain', '+group2:domain', ...] -}; - -const groupProfiles = { -// $groupId: { -// avatar_url: 'mxc://...' -// } -}; - -// Represents all unsettled promises to retrieve the groups for each userId. When a promise -// is settled, it is deleted from this object. -const usersPending = { -// $userId: { -// prom: Promise -// resolve: () => {} -// reject: () => {} -// } -}; - -let debounceTimeoutID; -function getPublicisedGroupsCached(matrixClient, userId) { - if (userGroups[userId]) { - return Promise.resolve(userGroups[userId]); - } - - // Bulk lookup ongoing, return promise to resolve/reject - if (usersPending[userId]) { - return usersPending[userId].prom; - } - - usersPending[userId] = {}; - usersPending[userId].prom = new Promise((resolve, reject) => { - usersPending[userId].resolve = resolve; - usersPending[userId].reject = reject; - }).then((groups) => { - userGroups[userId] = groups; - setTimeout(() => { - delete userGroups[userId]; - }, USER_GROUPS_CACHE_BUST_MS); - return userGroups[userId]; - }).catch((err) => { - throw err; - }).finally(() => { - delete usersPending[userId]; - }); - - // This debounce will allow consecutive requests for the public groups of users that - // are sent in intervals of < BULK_REQUEST_DEBOUNCE_MS to be batched and only requested - // when no more requests are received within the next BULK_REQUEST_DEBOUNCE_MS. The naive - // implementation would do a request that only requested the groups for `userId`, leading - // to a worst and best case of 1 user per request. This implementation's worst is still - // 1 user per request but only if the requests are > BULK_REQUEST_DEBOUNCE_MS apart and the - // best case is N users per request. - // - // This is to reduce the number of requests made whilst trading off latency when viewing - // a Flair component. - if (debounceTimeoutID) clearTimeout(debounceTimeoutID); - debounceTimeoutID = setTimeout(() => { - batchedGetPublicGroups(matrixClient); - }, BULK_REQUEST_DEBOUNCE_MS); - - return usersPending[userId].prom; -} - -async function batchedGetPublicGroups(matrixClient) { - // Take the userIds from the keys of usersPending - const usersInFlight = Object.keys(usersPending); - let resp = { - users: [], - }; - try { - resp = await matrixClient.getPublicisedGroups(usersInFlight); - } catch (err) { - // Propagate the same error to all usersInFlight - usersInFlight.forEach((userId) => { - usersPending[userId].reject(err); - }); - return; - } - const updatedUserGroups = resp.users; - usersInFlight.forEach((userId) => { - usersPending[userId].resolve(updatedUserGroups[userId] || []); - }); -} - -async function getGroupProfileCached(matrixClient, groupId) { - if (groupProfiles[groupId]) { - return groupProfiles[groupId]; - } - - const profile = await matrixClient.getGroupProfile(groupId); - groupProfiles[groupId] = { - groupId, - avatarUrl: profile.avatar_url, - }; - setTimeout(() => { - delete groupProfiles[groupId]; - }, GROUP_PROFILES_CACHE_BUST_MS); - - return groupProfiles[groupId]; -} class FlairAvatar extends React.Component { constructor() { @@ -193,14 +79,14 @@ export default class Flair extends React.Component { componentWillMount() { this._unmounted = false; - if (UserSettingsStore.isFeatureEnabled('feature_groups') && groupSupport) { + if (UserSettingsStore.isFeatureEnabled('feature_groups') && FlairStore.groupSupport()) { this._generateAvatars(); } this.context.matrixClient.on('RoomState.events', this.onRoomStateEvents); } onRoomStateEvents(event) { - if (event.getType() === 'm.room.related_groups' && groupSupport) { + if (event.getType() === 'm.room.related_groups' && FlairStore.groupSupport()) { this._generateAvatars(); } } @@ -210,7 +96,7 @@ export default class Flair extends React.Component { for (const groupId of groups) { let groupProfile = null; try { - groupProfile = await getGroupProfileCached(this.context.matrixClient, groupId); + groupProfile = await FlairStore.getGroupProfileCached(this.context.matrixClient, groupId); } catch (err) { console.error('Could not get profile for group', groupId, err); } @@ -220,24 +106,13 @@ export default class Flair extends React.Component { } async _generateAvatars() { - let groups; - try { - groups = await getPublicisedGroupsCached(this.context.matrixClient, this.props.userId); - } catch (err) { - // Indicate whether the homeserver supports groups - if (err.errcode === 'M_UNRECOGNIZED') { - console.warn('Cannot display flair, server does not support groups'); - groupSupport = false; - // Return silently to avoid spamming for non-supporting servers - return; - } - console.error('Could not get groups for user', this.props.userId, err); - } + let groups = await FlairStore.getPublicisedGroupsCached(this.context.matrixClient, this.props.userId); if (this.props.roomId && this.props.showRelated) { const relatedGroupsEvent = this.context.matrixClient .getRoom(this.props.roomId) .currentState .getStateEvents('m.room.related_groups', ''); + console.info('relatedGroupsEvent', relatedGroupsEvent); const relatedGroups = relatedGroupsEvent ? relatedGroupsEvent.getContent().groups || [] : []; if (relatedGroups && relatedGroups.length > 0) { diff --git a/src/stores/FlairStore.js b/src/stores/FlairStore.js new file mode 100644 index 0000000000..e77dac5337 --- /dev/null +++ b/src/stores/FlairStore.js @@ -0,0 +1,156 @@ +/* +Copyright 2017 New Vector Ltd + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import EventEmitter from 'events'; +import Promise from 'bluebird'; + +const BULK_REQUEST_DEBOUNCE_MS = 200; + +// Does the server support groups? Assume yes until we receive M_UNRECOGNIZED. +// If true, flair can function and we should keep sending requests for groups and avatars. +let groupSupport = true; + +const USER_GROUPS_CACHE_BUST_MS = 1800000; // 30 mins +const GROUP_PROFILES_CACHE_BUST_MS = 1800000; // 30 mins + +/** + * Stores data used by + */ +class FlairStore extends EventEmitter { + constructor(matrixClient) { + super(); + this._matrixClient = matrixClient; + this._userGroups = { + // $userId: ['+group1:domain', '+group2:domain', ...] + }; + this._groupProfiles = { + // $groupId: { + // avatar_url: 'mxc://...' + // } + }; + this._usersPending = { + // $userId: { + // prom: Promise + // resolve: () => {} + // reject: () => {} + // } + }; + + // this._fetchRooms(); + + this._debounceTimeoutID = null; + } + + groupSupport() { + return groupSupport; + } + + getPublicisedGroupsCached(matrixClient, userId) { + if (this._userGroups[userId]) { + return Promise.resolve(this._userGroups[userId]); + } + + // Bulk lookup ongoing, return promise to resolve/reject + if (this._usersPending[userId]) { + return this._usersPending[userId].prom; + } + + this._usersPending[userId] = {}; + this._usersPending[userId].prom = new Promise((resolve, reject) => { + this._usersPending[userId].resolve = resolve; + this._usersPending[userId].reject = reject; + }).then((groups) => { + this._userGroups[userId] = groups; + setTimeout(() => { + delete this._userGroups[userId]; + }, USER_GROUPS_CACHE_BUST_MS); + return this._userGroups[userId]; + }).catch((err) => { + // Indicate whether the homeserver supports groups + if (err.errcode === 'M_UNRECOGNIZED') { + console.warn('Cannot display flair, server does not support groups'); + groupSupport = false; + // Return silently to avoid spamming for non-supporting servers + return; + } + console.error('Could not get groups for user', this.props.userId, err); + throw err; + }).finally(() => { + delete this._usersPending[userId]; + }); + + // This debounce will allow consecutive requests for the public groups of users that + // are sent in intervals of < BULK_REQUEST_DEBOUNCE_MS to be batched and only requested + // when no more requests are received within the next BULK_REQUEST_DEBOUNCE_MS. The naive + // implementation would do a request that only requested the groups for `userId`, leading + // to a worst and best case of 1 user per request. This implementation's worst is still + // 1 user per request but only if the requests are > BULK_REQUEST_DEBOUNCE_MS apart and the + // best case is N users per request. + // + // This is to reduce the number of requests made whilst trading off latency when viewing + // a Flair component. + if (this._debounceTimeoutID) clearTimeout(this._debounceTimeoutID); + this._debounceTimeoutID = setTimeout(() => { + this._batchedGetPublicGroups(matrixClient); + }, BULK_REQUEST_DEBOUNCE_MS); + + return this._usersPending[userId].prom; + } + + async _batchedGetPublicGroups(matrixClient) { + // Take the userIds from the keys of this._usersPending + const usersInFlight = Object.keys(this._usersPending); + let resp = { + users: [], + }; + try { + resp = await matrixClient.getPublicisedGroups(usersInFlight); + } catch (err) { + // Propagate the same error to all usersInFlight + usersInFlight.forEach((userId) => { + this._usersPending[userId].reject(err); + }); + return; + } + const updatedUserGroups = resp.users; + usersInFlight.forEach((userId) => { + this._usersPending[userId].resolve(updatedUserGroups[userId] || []); + }); + } + + async getGroupProfileCached(matrixClient, groupId) { + if (this._groupProfiles[groupId]) { + return this._groupProfiles[groupId]; + } + + const profile = await matrixClient.getGroupProfile(groupId); + this._groupProfiles[groupId] = { + groupId, + avatarUrl: profile.avatar_url, + }; + setTimeout(() => { + delete this._groupProfiles[groupId]; + }, GROUP_PROFILES_CACHE_BUST_MS); + + return this._groupProfiles[groupId]; + } +} + +let singletonFlairStore = null; +if (!singletonFlairStore) { + singletonFlairStore = new FlairStore(); +} +module.exports = singletonFlairStore; From 71443e9b94a41a46accea5fce42fdcccd8da1dad Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 19 Oct 2017 10:34:24 +0100 Subject: [PATCH 2/3] Remove logs comments --- src/components/views/elements/Flair.js | 1 - src/stores/FlairStore.js | 2 -- 2 files changed, 3 deletions(-) diff --git a/src/components/views/elements/Flair.js b/src/components/views/elements/Flair.js index f0bfd817dd..7dc3070bc6 100644 --- a/src/components/views/elements/Flair.js +++ b/src/components/views/elements/Flair.js @@ -112,7 +112,6 @@ export default class Flair extends React.Component { .getRoom(this.props.roomId) .currentState .getStateEvents('m.room.related_groups', ''); - console.info('relatedGroupsEvent', relatedGroupsEvent); const relatedGroups = relatedGroupsEvent ? relatedGroupsEvent.getContent().groups || [] : []; if (relatedGroups && relatedGroups.length > 0) { diff --git a/src/stores/FlairStore.js b/src/stores/FlairStore.js index e77dac5337..11823f6761 100644 --- a/src/stores/FlairStore.js +++ b/src/stores/FlairStore.js @@ -49,8 +49,6 @@ class FlairStore extends EventEmitter { // } }; - // this._fetchRooms(); - this._debounceTimeoutID = null; } From 25d14af616d983929a79522e267349054de382fd Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 19 Oct 2017 12:00:03 +0100 Subject: [PATCH 3/3] Export a global.singletonFlairStore to allow cross-project singleton --- src/stores/FlairStore.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/stores/FlairStore.js b/src/stores/FlairStore.js index 11823f6761..ecb1bcd15c 100644 --- a/src/stores/FlairStore.js +++ b/src/stores/FlairStore.js @@ -147,8 +147,7 @@ class FlairStore extends EventEmitter { } } -let singletonFlairStore = null; -if (!singletonFlairStore) { - singletonFlairStore = new FlairStore(); +if (global.singletonFlairStore === undefined) { + global.singletonFlairStore = new FlairStore(); } -module.exports = singletonFlairStore; +export default global.singletonFlairStore;