Page MenuHomePhabricator
Differential D14493

[lib] Fix bug in ChatThreadItemLoaderCache for simultaneous loadMostRecent calls
ClosedPublic
Actions

Authored by ashoat on Mon, Mar 24, 7:11 AM.
Tags
None
Referenced Files
F5120417: D14493.diff
Thu, Apr 3, 7:45 AM
F5118121: D14493.diff
Wed, Apr 2, 10:32 PM
Unknown Object (File)
Tue, Apr 1, 2:15 PM
Unknown Object (File)
Mon, Mar 31, 4:23 PM
Unknown Object (File)
Mon, Mar 31, 10:34 AM
Unknown Object (File)
Sun, Mar 30, 10:12 PM
Unknown Object (File)
Sun, Mar 30, 9:14 PM
Unknown Object (File)
Sat, Mar 29, 8:20 PM
View All 18 Files
Subscribers
None

Details

Reviewers
tomek
angelika
Commits
rCOMMed79202ca6ba: [lib] Fix bug in ChatThreadItemLoaderCache for simultaneous loadMostRecent calls
Summary

If loadMostRecent is called multiple times, then this.loadingState will track the most recent call. We check this.loadingState here to make sure an earlier call does not set this.currentState, but we should break regardless to make sure that the earlier call returns once all of its items are resolved.

Test Plan

I tested this along with the rest of the stack by making sure that everything still worked on my physical device with a release build, and that performance was not degraded

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Revision Contents
Changeset List

PathSize
lib/
utils/
20 lines

Diff 47541

View Options

lib/utils/chat-thread-item-loader-cache.js

Show All 32 Lines (threadInfo, messageStore) => {
if (!thread || !viewerID || thread.messageIDs.length === 0) { if (!thread || !viewerID || thread.messageIDs.length === 0) {
return { return {
lastUpdatedAtLeastTime: threadInfo.creationTime, lastUpdatedAtLeastTime: threadInfo.creationTime,
lastUpdatedAtMostTime: threadInfo.creationTime, lastUpdatedAtMostTime: threadInfo.creationTime,
lastUpdatedTime: () => Promise.resolve(threadInfo.creationTime), lastUpdatedTime: () => Promise.resolve(threadInfo.creationTime),
}; };
} }
const mostRecentMessage = messageStore.messages[thread.messageIDs[0]];
const lastUpdatedAtMostTime =
mostRecentMessage?.time ?? threadInfo.creationTime;
// We default to the most recent message time. // We default to the most recent message time.
// - This default will only be relevant in the case that // - This default will only be relevant in the case that
// getLastUpdatedTime is implemented for every message in the loop // getLastUpdatedTime is implemented for every message in the loop
// below, and doesn't return a number for any of them. Otherwise // below, and doesn't return a number for any of them. Otherwise
// lastUpdatedAtLeastTime will be set to the most recent message that // lastUpdatedAtLeastTime will be set to the most recent message that
// returns a number, or whose message spec doesn't implement // returns a number, or whose message spec doesn't implement
// getLastUpdatedTime. // getLastUpdatedTime.
// - We default to the most recent message time because the best // - We default to the most recent message time because the best
// alternative is the thread creation time, but this is likely very // alternative is the thread creation time, but this is likely very
// far in the past. // far in the past.
const mostRecentMessage = messageStore.messages[thread.messageIDs[0]]; let lastUpdatedAtLeastTime = lastUpdatedAtMostTime;
let lastUpdatedAtLeastTime =
mostRecentMessage?.time ?? threadInfo.creationTime;
const getLastUpdatedTimeParams = { const getLastUpdatedTimeParams = {
threadInfo, threadInfo,
viewerID, viewerID,
getMessageAuthor: (messageID: string) => getMessageAuthor: (messageID: string) =>
baseGetMessageAuthor(messageID, messageStore), baseGetMessageAuthor(messageID, messageStore),
}; };
let lastUpdatedTime: ?() => Promise<?number>; let lastUpdatedTime: ?() => Promise<?number>;
let highestTimestamp: ?number;
for (const messageID of thread.messageIDs) { for (const messageID of thread.messageIDs) {
const messageInfo = messageStore.messages[messageID]; const messageInfo = messageStore.messages[messageID];
if (!messageInfo) { if (!messageInfo) {
continue; continue;
} }
// We call getLastUpdatedTime on the message spec. It can return either // We call getLastUpdatedTime on the message spec. It can return either
// ?number or () => Promise<?number>. If the message spec doesn't // ?number or () => Promise<?number>. If the message spec doesn't
// implement getLastUpdatedTime, then we default to messageInfo.time. // implement getLastUpdatedTime, then we default to messageInfo.time.
const { getLastUpdatedTime } = messageSpecs[messageInfo.type]; const { getLastUpdatedTime } = messageSpecs[messageInfo.type];
const lastUpdatedTimeResult = getLastUpdatedTime const lastUpdatedTimeResult = getLastUpdatedTime
? getLastUpdatedTime(messageInfo, getLastUpdatedTimeParams) ? getLastUpdatedTime(messageInfo, getLastUpdatedTimeParams)
: messageInfo.time; : messageInfo.time;
// We only need to consider the first positive number result because
// thread.messageIDs is ordered chronologically.
if (
!highestTimestamp &&
lastUpdatedTimeResult &&
typeof lastUpdatedTimeResult === 'number'
) {
highestTimestamp = lastUpdatedTimeResult;
}
// We rely on the fact that thread.messageIDs is ordered chronologically // We rely on the fact that thread.messageIDs is ordered chronologically
// (newest first) to chain together lastUpdatedTime. An older message's // (newest first) to chain together lastUpdatedTime. An older message's
// lastUpdatedTime is only considered if all of the newer messages // lastUpdatedTime is only considered if all of the newer messages
// return falsey. // return falsey.
const prevLastUpdatedTime = lastUpdatedTime; const prevLastUpdatedTime = lastUpdatedTime;
lastUpdatedTime = async () => { lastUpdatedTime = async () => {
if (prevLastUpdatedTime) { if (prevLastUpdatedTime) {
const earlierChecks = await prevLastUpdatedTime(); const earlierChecks = await prevLastUpdatedTime();
Show All 27 Lines (threadInfo, messageStore) => {
const earlierChecks = await lastUpdatedTime(); const earlierChecks = await lastUpdatedTime();
if (earlierChecks) { if (earlierChecks) {
return earlierChecks; return earlierChecks;
} }
} }
return lastUpdatedAtLeastTime; return lastUpdatedAtLeastTime;
}; };
const lastUpdatedAtMostTime = highestTimestamp ?? threadInfo.creationTime;
return { return {
lastUpdatedAtLeastTime, lastUpdatedAtLeastTime,
lastUpdatedAtMostTime, lastUpdatedAtMostTime,
lastUpdatedTime: lastUpdatedWithFallback, lastUpdatedTime: lastUpdatedWithFallback,
}; };
}, },
[viewerID, baseGetMessageAuthor], [viewerID, baseGetMessageAuthor],
); );
} }
export { useGetLastUpdatedTimes }; export { useGetLastUpdatedTimes };