Repository navigation
Indexeddb: transaction lifecycle - #41508
Merged
Merged
Conversation
|
🔨 Triggering try run (#20476718624) for Linux (WPT) |
Taym95
force-pushed
the
IndexedDB-transaction-lifecycle
branch
from
December 24, 2025 02:43
4d2ac3a to
82b8293
Compare
|
|
Taym95
force-pushed
the
IndexedDB-transaction-lifecycle
branch
from
December 24, 2025 03:01
82b8293 to
191b390
Compare
|
🔨 Triggering try run (#20476995178) for Linux (WPT) |
|
Test results for linux-wpt from try job (#20476995178): Flaky unexpected result (32)
Stable unexpected results that are known to be intermittent (25)
Stable unexpected results (18)
|
|
|
gterzian
reviewed
Dec 24, 2025
Member
There was a problem hiding this comment.
So I think it's a good start but I think some structural pieces are missing.
At a high-level(mostly for my own understanding):
When a transaction is created, it is set to active. Then at the next microtask it is set to inactive. However, for each even that fires after that for a request, it is set back to active. After the event has been fired and control is back to Rust, it should be set back to inactive.
The concept of "starting" a transaction I think is mostly a backend thing, even though the spec queues a task for it. Basically, while a transaction is active, requests can be made against it, but those requests should only start when the transaction can be started.
I added some comments here and there about structural improvements. Mostly about doing something via a round-trip with the backend as oppposed to directly in script.
| if mode != IDBTransactionMode::Versionchange { | ||
| if let Some(event_loop_id) = ScriptEventLoopId::installed() { | ||
| // https://w3c.github.io/IndexedDB/#transaction-concept | ||
| // A transaction optionally has a cleanup event loop which is an event loop. |
Member
There was a problem hiding this comment.
I think this is just Step 8: Set transaction’s cleanup event loop to the current event loop. of https://w3c.github.io/IndexedDB/#dom-idbdatabase-transaction
And IDBTransactionMode::Versionchange cannot be created manually since it is an internal "special" transaction.
Taym95
force-pushed
the
IndexedDB-transaction-lifecycle
branch
from
February 5, 2026 00:43
191b390 to
69a546f
Compare
|
🔨 Triggering try run (#21694176738) for Linux (WPT) |
|
Test results for linux-wpt from try job (#21694176738): Flaky unexpected result (40)
Stable unexpected results that are known to be intermittent (36)
Stable unexpected results (42)
|
|
|
Taym95
force-pushed
the
IndexedDB-transaction-lifecycle
branch
2 times, most recently
from
February 5, 2026 13:42
c45d385 to
48533f9
Compare
gterzian
reviewed
Feb 5, 2026
gterzian
reviewed
Feb 5, 2026
|
🔨 Triggering try run (#21734429433) for Linux (WPT) |
|
Test results for linux-wpt from try job (#21734429433): Flaky unexpected result (47)
Stable unexpected results that are known to be intermittent (33)
Stable unexpected results (61)
|
|
|
Signed-off-by: Taym Haddadi <haddadi.taym@gmail.com>
Collaborator
|
🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync. |
Signed-off-by: Taym Haddadi <haddadi.taym@gmail.com>
Collaborator
|
🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync. |
Member
Author
|
@janvarga do you think you can have look at this PR? |
Signed-off-by: Taym Haddadi <haddadi.taym@gmail.com>
Member
Yes, I’ve started looking at it. It’s a fairly large PR, and I was planning to finish my review tomorrow. I don’t want to block landing unless you’re okay waiting until then. |
Member
Author
Yes we could wait. |
Collaborator
|
🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync. |
Collaborator
|
🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync. |
janvarga
reviewed
Feb 18, 2026
janvarga
left a comment
Member
There was a problem hiding this comment.
Submitting some comments, I should be done with the review in couple of minutes.
| self.global() | ||
| .get_indexeddb() | ||
| .register_indexeddb_transaction(&transaction); | ||
| transaction.set_registered_in_global(); |
Member
There was a problem hiding this comment.
It looks like register_indexeddb_transaction/unregister_indexeddb_transaction could also set the flag (registered_in_global) and be a bit less error prone?
janvarga
approved these changes
Feb 18, 2026
janvarga
left a comment
Member
There was a problem hiding this comment.
Nice work!
This is a fairly large set of changes, so I didn’t manage to verify every single detail, but from a high-level perspective this looks good to me (modulo a few inline comments and questions).
| } | ||
|
|
||
| pub(crate) fn clear_upgrade_transaction(&self, transaction: &IDBTransaction) { | ||
| let Some(current) = self.upgrade_transaction.get() else { |
Member
There was a problem hiding this comment.
This looks a bit too defensive, can it happen that the passed transaction doesn't match self.upgrade_transaction or that self.upgrade_transaction is not set?
If so, maybe rename the function to indicate that?
Member
Author
There was a problem hiding this comment.
Either I renamed to clear_upgrade_transaction_if_matches or I change implementation to debug_assert! so function be called in contexts where it must match.
| }, | ||
| }) | ||
| }; | ||
| if mode != IDBTransactionMode::Versionchange { |
Member
There was a problem hiding this comment.
Step 6 actually says "If mode is not "readonly" or "readwrite", throw a TypeError."
So you can change this check to implement Step 6 or at least add a TODO comment.
Member
Author
There was a problem hiding this comment.
Indeed it was an over-sign on my side. Thank you.
Member
Author
There was a problem hiding this comment.
This made another test pass.
| connections: | ||
| DomRefCell<HashMapTracedValues<DBName, HashMapTracedValues<Uuid, Dom<IDBOpenDBRequest>>>>, | ||
|
|
||
| /// <https://www.w3.org/TR/IndexedDB-2/#transaction> |
Member
There was a problem hiding this comment.
NIT, it would be better to refer to the latest spec or at least the version 3
| // transactions on the global and deactivating them at the microtask checkpoint. | ||
| let snapshot: Vec<DomRoot<IDBTransaction>> = { | ||
| let mut transactions = self.indexeddb_transactions.borrow_mut(); | ||
| transactions.retain(|txn| !txn.is_finished()); |
Member
There was a problem hiding this comment.
Hm, is this retain needed ? Does it mean that transactions are not always unregistered when finished?
Member
Author
There was a problem hiding this comment.
I added the retain(!is_finished) as a defensive cleanup for delayed unregister, I can keep it and add a comment explaining the race/window.
| self.indexeddb_transactions | ||
| .borrow_mut() | ||
| .retain(|txn| !txn.is_finished()); | ||
| true |
Member
There was a problem hiding this comment.
Maybe there's a good reason for the snapshot, I guess it's not possible to avoid multiple iterations, maybe once extract_if is available?
Member
Author
There was a problem hiding this comment.
So the snapshot gives me a clean separation:
-
Borrow list briefly ->> clone roots into
Vec<DomRoot<_>> -
Drop borrow
-
Work on the snapshot safely (calls may re enter)
| let task_source = global | ||
| .task_manager() | ||
| .database_access_task_source() | ||
| .dom_manipulation_task_source() |
Member
There was a problem hiding this comment.
Maybe someone already asked about this (sorry if I overlooked it), but why the task source is changed here and elsewhere?
Member
Author
There was a problem hiding this comment.
this should be database_access_task_source, I will undo it
| // finished. | ||
| pending_request_count: Cell<usize>, | ||
| next_request_id: Cell<u64>, | ||
| handled_next_unhandled_id: Cell<u64>, |
Member
Author
There was a problem hiding this comment.
Renamed handled_next_unhandled_id to next_unhandled_request_id and added a short comment describing the invariant.
| Err(_err) => { | ||
| // TODO: Map backend commit error to appropriate DOMException. | ||
| this.initiate_abort( | ||
| Error::QuotaExceeded { |
Member
There was a problem hiding this comment.
I'm a bit surprised by using QuotaExceeded here I guess it's only temporary.
Member
Author
There was a problem hiding this comment.
I will use Error::Operation for now.
| } | ||
|
|
||
| pub(crate) fn maybe_commit(&self) { | ||
| // https://w3c.github.io/IndexedDB/#transaction-lifetime |
Member
There was a problem hiding this comment.
I have a gut feeling that this logic might need to be in the backend in the end, especially once the explicit commit() is implemented.
| transactions: FxHashMap<u64, KvsTransaction>, | ||
| next_created_seq: u64, | ||
| txn_info: FxHashMap<u64, TxnInfo>, | ||
| queued_readwrite: VecDeque<u64>, |
Member
There was a problem hiding this comment.
Given these vectors, etc. I'm a bit worried that the scheduling can be slow with large numbers of transactions, but that can be optimized later.
Member
Author
There was a problem hiding this comment.
you are right, I will introduce queued_readwrite_set and queued_readonly_set for fast membership checks + de-dup for queued_*
…heduling - Keep a defensive prune of finished transactions before snapshotting in cleanup, since unregister can happen in queued tasks. - Rework script-side transaction tracking to be keyed by db_name to avoid linear scans when looking up a txn to maybe-commit. - Use dom_manipulation_task_source where required to preserve ordering/expectations; document why database_access_task_source caused test failures. - Rename handled_next_unhandled_id to handled_next_unhandled_request_id for clarity (aligns with backend terminology). - Treat backend commit failure mapping as temporary; avoid implying QuotaExceeded unless the backend reports it. - Improve backend scheduling performance by avoiding O(n) membership checks on queues (keep VecDeque for FIFO order and add a set for O(1) contains). Signed-off-by: Taym Haddadi <haddadi.taym@gmail.com>
|
🔨 Triggering try run (#22164458579) for Linux (WPT) |
Collaborator
|
🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync. |
Signed-off-by: Taym Haddadi <haddadi.taym@gmail.com>
Collaborator
|
🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync. |
|
Test results for linux-wpt from try job (#22164458579): Flaky unexpected result (24)
Stable unexpected results that are known to be intermittent (20)
Stable unexpected results (5)
|
|
|
no-op success so script side completion does not spuriously abort Signed-off-by: Taym Haddadi <haddadi.taym@gmail.com>
|
🔨 Triggering try run (#22176319213) for Linux (WPT) |
Collaborator
|
🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync. |
|
Test results for linux-wpt from try job (#22176319213): Flaky unexpected result (24)
Stable unexpected results that are known to be intermittent (15)
|
|
✨ Try run (#22176319213) succeeded. |
Member
Author
Contributor
|
Congrats on landing this work! |
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
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.
Indexeddb: transaction lifecycle
Testing: more Indexeddb test should pass
part of #40983