Indexeddb: connection lifecycle#42082
Merged
gterzian merged 15 commits intoservo:mainfrom Feb 2, 2026
Merged
Conversation
gterzian
force-pushed
the
indexeddb_track_connections_2
branch
from
9cd2c18 to
9a7f9ca
Compare
|
🔨 Triggering try run (#21292559435) for Linux (WPT) |
gterzian
force-pushed
the
indexeddb_track_connections_2
branch
from
a12892a to
561159a
Compare
|
🔨 Triggering try run (#21293293663) for Linux (WPT) |
|
|
|
Test results for linux-wpt from try job (#21293293663): Flaky unexpected result (40)
Stable unexpected results that are known to be intermittent (28)
Stable unexpected results (25)
|
|
|
gterzian
force-pushed
the
indexeddb_track_connections_2
branch
from
addcafb to
f862459
Compare
|
🔨 Triggering try run (#21344975552) for Linux (WPT) |
|
Test results for linux-wpt from try job (#21344975552): Flaky unexpected result (26)
Stable unexpected results that are known to be intermittent (32)
Stable unexpected results (14)
|
|
|
gterzian
force-pushed
the
indexeddb_track_connections_2
branch
4 times, most recently
from
9de5f92 to
3e77d3f
Compare
|
🔨 Triggering try run (#21362608695) for Linux (WPT) |
gterzian
force-pushed
the
indexeddb_track_connections_2
branch
from
3e77d3f to
4afb239
Compare
|
|
|
🔨 Triggering try run (#21362948634) for Linux (WPT) |
servo-highfive
added
the
S-awaiting-review
gterzian
force-pushed
the
indexeddb_track_connections_2
branch
from
328656c to
9637be4
Compare
gterzian
force-pushed
the
indexeddb_track_connections_2
branch
from
9637be4 to
406f207
Compare
Signed-off-by: gterzian <[email protected]>
gterzian
force-pushed
the
indexeddb_track_connections_2
branch
from
406f207 to
054a907
Compare
Signed-off-by: gterzian <[email protected]>
Taym95
approved these changes
Jan 28, 2026
Member
Taym95
left a comment
Taym95
left a comment
There was a problem hiding this comment.
small remark: maybe we should not add a connection id to pending_ unless we have a proven path that will later remove, If delivery fails, we resolve immediately by removing the connection or aborting the request.
| // named versionchange at entry with db’s version and version. | ||
| if conn | ||
| .sender | ||
| .send(ConnectionMsg::VersionChange { |
Member
There was a problem hiding this comment.
will backend stall forever if it can’t deliver VersionChange to script ?
Member
Author
There was a problem hiding this comment.
Yes that's a good point, but instead of handling it here I think we should have a separate workflow for aborting/closing everything when a global unloads; I've added an item to #40983
Member
There was a problem hiding this comment.
Right, I think I asked about this some time ago already.
Essentially, once we detect that the channel (in the multi-process case) is no longer sendable, everything needs to be torn down, and doing that correctly is usually non-trivial. So I agree this is best handled separately, once the main architecture is in place.
| /// We store the open request, which contains the connection. | ||
| /// TODO: remove when we are sure they are not needed anymore. | ||
| connections: | ||
| DomRefCell<HashMapTracedValues<DBName, HashMapTracedValues<Uuid, Dom<IDBOpenDBRequest>>>>, |
Member
There was a problem hiding this comment.
question: why not keep a registry keyed by (db_name, connection_id) that stores the IDBDatabase not the request, HashMap<(DBName, Uuid), Dom<IDBDatabase>> and use it for VersionChange delivery then let requests be removable after success/error without risking upgrade coordination ?
Member
Author
There was a problem hiding this comment.
We could have some sort of struct with requests and connections; now we just use the request for that which does mean it is kept alive for longer because at some point we only need the connection. Not completely sure what it should look like however. Something to look into later? (deleting a db still needs to be integrated with the various events firing also)
Member
There was a problem hiding this comment.
Right, no need to keep requests alive for so long, but for now, it's ok I think (at the moment, the architecture is more important than cleanup/optimizations, worth a TODO though).
servo-highfive
removed
the
S-awaiting-review
Member
|
Let me take a quick look ... |
janvarga
reviewed
Jan 29, 2026
Member
janvarga
left a comment
janvarga
left a comment
There was a problem hiding this comment.
Only NITs so far, but still looking at components/storage/indexeddb/mod.rs ...
|
|
||
| #[no_trace] | ||
| #[ignore_malloc_size_of = "Uuid"] | ||
| id: Uuid, |
Member
There was a problem hiding this comment.
NIT, a bit heavy weight as on the open request IIRC
|
|
||
| if receiver.recv().is_err() { | ||
| warn!("Database close failed in idb thread"); | ||
| }; |
Member
There was a problem hiding this comment.
Yes, this used to be synchronous, but I don’t think that’s what the spec intends, nor how other implementations behave.
NIT: The comments above probably need to be adjusted/removed (Step 3 can be removed I think)
BTW, The forced flag, that's more about the case when the backed would ask clients to do a forced close, for example when backed wants to clear data for entire origin.
| /// We store the open request, which contains the connection. | ||
| /// TODO: remove when we are sure they are not needed anymore. | ||
| connections: | ||
| DomRefCell<HashMapTracedValues<DBName, HashMapTracedValues<Uuid, Dom<IDBOpenDBRequest>>>>, |
Member
There was a problem hiding this comment.
Right, no need to keep requests alive for so long, but for now, it's ok I think (at the moment, the architecture is more important than cleanup/optimizations, worth a TODO though).
| /// Messaging used in the context of connection lifecycle management. | ||
| pub enum ConnectionMsg { | ||
| /// Error if a DB is opened for a version | ||
| /// lower than the current db version. |
Member
There was a problem hiding this comment.
NIT: Fits one line (here and below)
janvarga
approved these changes
Jan 29, 2026
Member
janvarga
left a comment
janvarga
left a comment
There was a problem hiding this comment.
I didn’t find anything major. Overall this looks pretty good, and it seems you understood the spec very well. Thanks!
| pending_upgrade: Option<VersionUpgrade>, | ||
|
|
||
| /// This request is pending on these connections to close. | ||
| pending_close: HashSet<Uuid>, |
Member
There was a problem hiding this comment.
This is more about future development, but in theory, instead of storing IDs everywhere, we could keep references to the connections themselves (e.g. Rc or similar).
Is this something we’re intentionally avoiding, or could it be a direction we consider later on?
(I use such refs in some of my WIP PRs)
Member
Author
There was a problem hiding this comment.
Yes we could use references; I've had a quick look and the algo just use the id so far, for example by sending a message with the id, so keeping a ref to the connection would only result in using the connection to get the id.
| version: u64, | ||
|
|
||
| /// <https://w3c.github.io/IndexedDB/#connection-close-pending-flag> | ||
| close_pending: bool, |
Member
There was a problem hiding this comment.
NIT: Now when I see it here, I think the flag on IDBDatabase could be renamed from closing to close_pending.
Member
Author
There was a problem hiding this comment.
yes defenitely, but it's pre-existing this PR; I've added an item to #40983
|
|
||
| /// <https://w3c.github.io/IndexedDB/#connection> | ||
| connections: HashMap<IndexedDBDescription, HashSet<Connection>>, | ||
| connections: HashMap<IndexedDBDescription, HashMap<Uuid, Connection>>, |
Member
There was a problem hiding this comment.
Ok, so this fixes the issue from previous PR.
| } | ||
| } | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
Yeah, this part is quite complicated and it looks it's correctly implemented.
| // named versionchange at entry with db’s version and version. | ||
| if conn | ||
| .sender | ||
| .send(ConnectionMsg::VersionChange { |
Member
There was a problem hiding this comment.
Right, I think I asked about this some time ago already.
Essentially, once we detect that the channel (in the multi-process case) is no longer sendable, everything needs to be torn down, and doing that correctly is usually non-trivial. So I agree this is best handled separately, once the main architecture is in place.
| // Step 2: If the forced flag is true, | ||
| // then for each transaction created using connection | ||
| // run abort a transaction with transaction and newly created "AbortError" DOMException. | ||
| // Step 3: Wait for all transactions created using connection to complete. |
Member
There was a problem hiding this comment.
Right, waiting for all transactions to finish is next major thing which needs to be done (before continuing with requested upgrade).
| let global = self.global(); | ||
| let this = Trusted::new(self); | ||
| global.task_manager().database_access_task_source().queue( | ||
| task!(send_versionchange_notification: move || { |
Contributor
There was a problem hiding this comment.
Wasn't this being done correctly previously, by being a task instead of synchronous?
Member
Author
There was a problem hiding this comment.
In the current code the method is called from a task already.
| } | ||
|
|
||
| /// <https://w3c.github.io/IndexedDB/#closing-connection> | ||
| fn close_database(&mut self, origin: ImmutableOrigin, id: Uuid, name: String) { |
Contributor
There was a problem hiding this comment.
I think it should be possible to mark the connection's close_pending flag for step 1 right? IIRC it's in the connection struct
Member
Author
There was a problem hiding this comment.
Yes I actually realize the flag is not used here, what I'm doing now is just removing the connection from the map. Maybe we should remove the flag from the connection struct, but then again perhaps I am removing the connection from the map too soon and this will become obvious as part of transaction lifecycle. cc @Taym95
I've added an item to #40983
| global: &GlobalScope, | ||
| name: String, | ||
| version: u64, | ||
| upgraded: bool, |
Contributor
There was a problem hiding this comment.
Since you're passing in false for upgraded in all cases but one, does it really make sense to keep it here?
Member
Author
There was a problem hiding this comment.
We need a way to pass either false or true in the final connection message handling.
Signed-off-by: gterzian <[email protected]>
Signed-off-by: gterzian <[email protected]>
servo-highfive
added
the
S-awaiting-review
gterzian
commented
Jan 30, 2026
Member
Author
gterzian
left a comment
gterzian
left a comment
There was a problem hiding this comment.
Thank you all for the reviews, I think I have addressed all items.
| let global = self.global(); | ||
| let this = Trusted::new(self); | ||
| global.task_manager().database_access_task_source().queue( | ||
| task!(send_versionchange_notification: move || { |
Member
Author
There was a problem hiding this comment.
In the current code the method is called from a task already.
|
|
||
| if receiver.recv().is_err() { | ||
| warn!("Database close failed in idb thread"); | ||
| }; |
| global: &GlobalScope, | ||
| name: String, | ||
| version: u64, | ||
| upgraded: bool, |
Member
Author
There was a problem hiding this comment.
We need a way to pass either false or true in the final connection message handling.
| pending_upgrade: Option<VersionUpgrade>, | ||
|
|
||
| /// This request is pending on these connections to close. | ||
| pending_close: HashSet<Uuid>, |
Member
Author
There was a problem hiding this comment.
Yes we could use references; I've had a quick look and the algo just use the id so far, for example by sending a message with the id, so keeping a ref to the connection would only result in using the connection to get the id.
| version: u64, | ||
|
|
||
| /// <https://w3c.github.io/IndexedDB/#connection-close-pending-flag> | ||
| close_pending: bool, |
Member
Author
There was a problem hiding this comment.
yes defenitely, but it's pre-existing this PR; I've added an item to #40983
| } | ||
|
|
||
| /// <https://w3c.github.io/IndexedDB/#closing-connection> | ||
| fn close_database(&mut self, origin: ImmutableOrigin, id: Uuid, name: String) { |
Member
Author
There was a problem hiding this comment.
Yes I actually realize the flag is not used here, what I'm doing now is just removing the connection from the map. Maybe we should remove the flag from the connection struct, but then again perhaps I am removing the connection from the map too soon and this will become obvious as part of transaction lifecycle. cc @Taym95
I've added an item to #40983
Signed-off-by: gterzian <[email protected]>
arihant2math
approved these changes
Jan 30, 2026
servo-highfive
removed
the
S-awaiting-review
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Implement the full connection lifecycle, from opening and potentially upgrading a database, waiting on existing connections to close, firing the versionchange and blocked events, and updating pending open requests when connections close.
Testing: WPT
Fixes: Part of #40983