Changeset View
Standalone View
keyserver/src/socket/socket.js
Show First 20 Lines • Show All 71 Lines • ▼ Show 20 Lines | import { | ||||
newEntryQueryInputValidator, | newEntryQueryInputValidator, | ||||
verifyCalendarQueryThreadIDs, | verifyCalendarQueryThreadIDs, | ||||
} from '../responders/entry-responders.js'; | } from '../responders/entry-responders.js'; | ||||
import { handleAsyncPromise } from '../responders/handlers.js'; | import { handleAsyncPromise } from '../responders/handlers.js'; | ||||
import { | import { | ||||
fetchViewerForSocket, | fetchViewerForSocket, | ||||
extendCookieLifespan, | extendCookieLifespan, | ||||
createNewAnonymousCookie, | createNewAnonymousCookie, | ||||
isCookieMissingSignedIdentityKeysBlob, | |||||
} from '../session/cookies.js'; | } from '../session/cookies.js'; | ||||
import { Viewer } from '../session/viewer.js'; | import { Viewer } from '../session/viewer.js'; | ||||
import { commitSessionUpdate } from '../updaters/session-updaters.js'; | import { commitSessionUpdate } from '../updaters/session-updaters.js'; | ||||
import { assertSecureRequest } from '../utils/security-utils.js'; | import { assertSecureRequest } from '../utils/security-utils.js'; | ||||
import { | import { | ||||
checkInputValidator, | checkInputValidator, | ||||
checkClientSupported, | checkClientSupported, | ||||
policiesValidator, | policiesValidator, | ||||
▲ Show 20 Lines • Show All 449 Lines • ▼ Show 20 Lines | if (!sessionInitializationResult.sessionContinued) { | ||||
updatesResult, | updatesResult, | ||||
deltaEntryInfos: deltaEntryInfoResult.rawEntryInfos, | deltaEntryInfos: deltaEntryInfoResult.rawEntryInfos, | ||||
deletedEntryIDs: deltaEntryInfoResult.deletedEntryIDs, | deletedEntryIDs: deltaEntryInfoResult.deletedEntryIDs, | ||||
userInfos: values(userInfos), | userInfos: values(userInfos), | ||||
}, | }, | ||||
}); | }); | ||||
} | } | ||||
if (viewer.cookieID) { | |||||
ashoat: This check is not necessary. `viewer.cookieID` always returns a string, so you're just checking… | |||||
atulAuthorUnsubmitted Done Inline Actionsatul: Okay, will remove. Thought it was necessary check because:
{F427377} | |||||
const signedIdentityKeysBlobMissing = | |||||
await isCookieMissingSignedIdentityKeysBlob(viewer.cookieID); | |||||
ashoatUnsubmitted Not Done Inline ActionsOne easy way to improve Promise performance without bothering with Promise.all and the complexity it forces is to simply start the promise as soon as you can. Eg. const signedIdentityKeysBlobMissingPromise = isCookieMissingSignedIdentityKeysBlob(viewer.cookieID); can be moved to eg. line 469 ashoat: One easy way to improve Promise performance without bothering with `Promise.all` and the… | |||||
atulAuthorUnsubmitted Done Inline ActionsWill move atul: Will move | |||||
if (signedIdentityKeysBlobMissing) { | |||||
serverRequests.push({ | |||||
type: serverRequestTypes.SIGNED_IDENTITY_KEYS_BLOB, | |||||
}); | |||||
} | |||||
} | |||||
atulAuthorUnsubmitted Done Inline Actions
because if, for example, there was a clientResponse to a SIGNED_IDENTITY_KEYS_BLOB request: there would be a race condition between A. setting the signed_identity_keys column As a result I decided to sequence the isCookieMissingSignedIdentityKeysBlob check AFTER the processClientResponses check to ensure that the result from isCookieMissingSignedIdentityKeysBlob is current and "up-to-date." It's not the end of the world if isCookieMissingSignedIdentityKeysBlob gives us a stale NULL since we'll just request it from the client again, but figured this approach was more correct. I also didn't think it made sense to handle within the Promise.alls within the !sessionInitializationResult.sessionContinued block or the corresponding else block because we'd need to write the check out twice. There are definitely ways to pull promises={} outside of the conditional branches and "pull the common stuff out," but I thought it was simpler to just sequence isCookieMissingSignedIdentityKeysBlob after those blocks and concluded any overhead was minimal. IMPORTANT: This is my first time working with this part of the codebase so I'm less confident in these changes than I might be with other diffs. I tried to read through and understand things as best as I could, but please let me know if any of the assumptions listed above (or otherwise) need to be revisited or reconsidered. atul: 1. I thought that it made the most sense to handle this check and `serverRequest` "once" in… | |||||
if (serverRequests.length > 0 || clientResponses.length > 0) { | if (serverRequests.length > 0 || clientResponses.length > 0) { | ||||
// We send this message first since the STATE_SYNC triggers the client's | // We send this message first since the STATE_SYNC triggers the client's | ||||
// connection status to shift to "connected", and we want to make sure the | // connection status to shift to "connected", and we want to make sure the | ||||
// client responses are cleared from Redux before that happens | // client responses are cleared from Redux before that happens | ||||
responses.unshift({ | responses.unshift({ | ||||
type: serverSocketMessageTypes.REQUESTS, | type: serverSocketMessageTypes.REQUESTS, | ||||
responseTo: message.id, | responseTo: message.id, | ||||
payload: { serverRequests }, | payload: { serverRequests }, | ||||
▲ Show 20 Lines • Show All 270 Lines • Show Last 20 Lines |
This check is not necessary. viewer.cookieID always returns a string, so you're just checking if it's the empty string (which isn't possible)