-
-
Notifications
You must be signed in to change notification settings - Fork 590
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't swallow errors coming from the shareSession call #2908
Closed
Commits on Dec 8, 2022
-
Make sure that MegolmEncryption.setupPromise always resolves
The ensureOutboundSession users and modifies the setupPromise of the MegolmEncryption class. Some comments suggest that setupPromise will always resolve, in other words it should never contain a promise that will get rejected. Other comments also seem to suggest that the return value of ensureOutboundSession, a promise as well, may fail. The critical error here is that the promise that gets set as the next setupPromise, as well as the promise that ensureOutboundSession returns, is the same promise. It seems that the intention was for setupPromise to contain a promise that will always resolve to either `null` or `OutboundSessionInfo`. We can see that a couple of lines before we set setupPromise to its new value we construct a promise that logs and discards errors using the `Promise.catch()` method. The `Promise.catch()` method does not mutate the promise, instead it returns a new promise. The intention of the original author might have been to set the next setupPromise to the promise which `Promise.catch()` produces. This patch modifies the updating of setupPromise in the ensureOutboundSession so that setupPromise discards errors correctly. Using `>>=` to represent the promise chaining operation, setupPromise is now updated using the following logic: setupPromise = previousSetupPromise >>= setup >>= discardErrors
Configuration menu - View commit details
-
Copy full SHA for 146de50 - Browse repository at this point
Copy the full SHA 146de50View commit details -
Don't swallow up errors coming from the shareSession call
A call to ensureSession() has two steps: 1. prepareSession(), where an outbound group session might get created or rotated 2. shareSession(), where an outbound group session might get encrypted and queued up to be sent to other devices Both of those calls may mostly fail due to storage errors, yet only the errors from prepareSession get propagated to the caller. Errors from prepareSession will mean that you can't get an outbound group session so you can't encrypt an event. Errors from shareSession, especially if the error happens in the part where the to-device requests are queued up to be sent out, mean that other people will not be able to decrypt the events that will get encrypted using the outbound group session. Both of those cases are catastrophic, the second case is just much harder to debug, since the error happens on another device at some arbitrary point in the future. Let's just return the error instead, people can then retry and the storage issue might have been resolved, or at least the error becomes visible when it happens.
Configuration menu - View commit details
-
Copy full SHA for 752b612 - Browse repository at this point
Copy the full SHA 752b612View commit details -
Configuration menu - View commit details
-
Copy full SHA for 05d6bf3 - Browse repository at this point
Copy the full SHA 05d6bf3View commit details -
Configuration menu - View commit details
-
Copy full SHA for f0b726f - Browse repository at this point
Copy the full SHA f0b726fView commit details
Commits on Dec 9, 2022
-
Configuration menu - View commit details
-
Copy full SHA for 0b1a51e - Browse repository at this point
Copy the full SHA 0b1a51eView commit details -
Configuration menu - View commit details
-
Copy full SHA for 6c43e2c - Browse repository at this point
Copy the full SHA 6c43e2cView commit details -
Configuration menu - View commit details
-
Copy full SHA for 95d883c - Browse repository at this point
Copy the full SHA 95d883cView commit details -
Configuration menu - View commit details
-
Copy full SHA for 638a263 - Browse repository at this point
Copy the full SHA 638a263View commit details
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.