-
Notifications
You must be signed in to change notification settings - Fork 260
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
indexeddb: Update storage for inbound_group_sessions
#2885
Conversation
08d5192
to
63ae9f1
Compare
c8688ce
to
a4c46b1
Compare
Currently, querying for inbound group sessions which need backing up is very inefficient: we have to search through the whole list. Here, we change the way they are stored so that we can maintain an index of the ones that need a backup.
63ae9f1
to
0344d6a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2885 +/- ##
=======================================
Coverage 82.66% 82.66%
=======================================
Files 216 216
Lines 22182 22182
=======================================
Hits 18337 18337
Misses 3845 3845 ☔ View full report in Codecov by Sentry. |
inbound_group_sessions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing tests. Mostly asking for small clarifications here and there.
let session = InboundGroupSession::from_pickle(pickled_session) | ||
.map_err(|e| IndexeddbCryptoStoreError::CryptoStoreError(e.into()))?; | ||
|
||
// Although a "backed up" flag is stored inside the `data`, it is not maintained |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear what data
is referring to, here? (I suppose the idb_object.data
— maybe a sign that the data
field should be named something else)
Also: why not maintain the backed_up
flag when resetting the backups? At a first glance, it seems that resetting the backups is the least frequent, when deserializing an inbound group session would be quite frequent. And I'm wary of adding logic to a deserialization function; ideally we would not need to have to remember that we can't just plain deserialize-and-build-from-pickle as we consistently do elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear what data is referring to, here? (I suppose the idb_object.data — maybe a sign that the data field should be named something else)
Renamed, as below.
Also: why not maintain the
backed_up
flag when resetting the backups? At a first glance, it seems that resetting the backups is the least frequent, when deserializing an inbound group session would be quite frequent.
Well, because it would involve decrypting, deserializing, updating, re-serializing, and re-encrypting each of possibly hundreds of thousands of pickled sessions. I don't think that's a price worth paying to simplify the code a bit. (The decryption and encryption bits are particularly expensive.)
Ideally we would not need to have to remember that we can't just plain deserialize-and-build-from-pickle as we consistently do elsewhere.
I don't disagree, but any deserialization is necessarily going to happen via deserialize_inbound_group_session
. In other words, the awkwardness is constrained to one particular function, and I don't really see that we'd need to remember it elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Currently, querying for inbound group sessions which need backing up is very
inefficient: we have to search through the whole list.
Here, we change the way they are stored so that we can maintain an index of the
ones that need a backup.
Fixes: element-hq/element-web#26488
Fixes: #2877