Dedupe requests to fetch group profile data #1666
Conversation
src/stores/FlairStore.js
Outdated
| setTimeout(() => { | ||
| delete this._groupProfiles[groupId]; | ||
| delete this._groupProfilesPromise[groupId]; |
There was a problem hiding this comment.
why does this have to happen in the callback? can't it happen as soon as the promise returns?
There was a problem hiding this comment.
You would think that - I did. Empirically that didn't work because it means that a promise could be deleted whilst something is still awaiting it. It's a race because in one version of the resolution, the promise would be deleted.
In this new version I think I've inadvertently created a weird barrier where every resolution sets this._groupProfiles[groupId] and sets its own timeout. Maybe it shouldn't do that.....
There was a problem hiding this comment.
I don't see why it's a problem if this._groupProfilesPromise is cleared while something is waiting for the promise.
There was a problem hiding this comment.
Actually my last comment doesn't make sense because the await already has a reference to the promise, it doesn't matter if a reference to the same promise is remove from this._groupProfilesPromise.
src/stores/FlairStore.js
Outdated
| this._groupProfilesPromise[groupId] = matrixClient.getGroupProfile(groupId); | ||
| } | ||
|
|
||
| const profile = await this._groupProfilesPromise[groupId]; |
There was a problem hiding this comment.
you might want to think about what happens if the getGroupProfile request fails. I think you'll get stuck with a failed promise, which is sad-making.
There was a problem hiding this comment.
Actually, the error will be thrown as normal
There was a problem hiding this comment.
yes, but then every attempt to get the profile for this group will then fail immediately for the next half-hour, even if whatever network blip made the first request fail is fixed.
There was a problem hiding this comment.
Ah okay, I shall wrap it with a try/catch and delete the failed promise in the catch.
Also, delete the groupProfilePromise immediately after setting the group profile (the first if-statement will prevent a new request from being started).
lukebarnard1
force-pushed
the
luke/fix-duplicate-group-profile-requests
branch
from
0e2c1b6
to
3947a72
Compare
|
Travis seems to be failing due to nvm not being able to download the Node.js binary, and then deciding to build it from source. "Yey". (https://travis-ci.org/matrix-org/matrix-react-sdk/builds/324079818?utm_source=github_status&utm_medium=notification) |
|
Having restarted the build, it seems to have been made happy. |
src/stores/FlairStore.js
Outdated
| try { | ||
| profile = await this._groupProfilesPromise[groupId]; | ||
| } catch (e) { | ||
| // Don't retry, but allow a retry when the profile is next requested |
There was a problem hiding this comment.
do we perhaps want to log the exception here, just in case it's something unexpectedly bad?
There was a problem hiding this comment.
Yep, seems fair
|
lgtm mod comment |
Possibly fixes element-hq/element-web#5899