[WIP] Refactored webext-storage database close function #6424
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6424 +/- ##
==========================================
+ Coverage 49.15% 52.55% +3.39%
==========================================
Files 147 126 -21
Lines 13750 12862 -888
==========================================
Hits 6759 6759
+ Misses 6991 6103 -888 ☔ View full report in Codecov by Sentry. |
lougeniaC64
force-pushed
the
refactor-webext-storage
branch
4 times, most recently
from
39c9321
to
9595b1d
Compare
lougeniaC64
force-pushed
the
refactor-webext-storage
branch
from
9595b1d
to
2d0d734
Compare
There was a problem hiding this comment.
know this is still in draft, but I wanted to leave a bit of feedback to unblock you. This is looking great, @lougeniaC64; thanks again for making all those changes! This sets us up for adding the shutdown blocker in Firefox, and then we can re-land your Firefox patch and close the chapter on this migration!
I’m requesting changes specifically for WebExtStorageStore::close: I don’t think the Arc::try_unwrap(self.db.clone()) is going to do what we want. Everything else looks great, though, including the switch to .unchecked_transaction().
| WebExtStorageDb::Open(conn) => conn, | ||
| WebExtStorageDb::Closed => return Ok(()), | ||
| }; | ||
| conn.close().map_err(|(_, y)| Error::SqlError(y)) |
There was a problem hiding this comment.
🎉 Thanks for cleaning up the no-longer-needed forget(), too!
| } | ||
| } | ||
|
|
||
| impl Deref for StorageDb { |
There was a problem hiding this comment.
Thank you also for powering through updating all the call sites; I know it’s a chore!
I do like that we aren’t relying on the Deref coercion anymore, as a nice side effect of your patch! Rust likes those for smart pointer and “guard” types, but StorageDb doesn’t quite fit into that bucket.
| let tx = db.transaction()?; | ||
| let db = new_mem_db(); | ||
| let conn = db.get_connection().expect("should retrieve connection"); | ||
| let tx = conn.unchecked_transaction()?; |
There was a problem hiding this comment.
👍 This looks good to me, too; I don’t think it’s worth adding a separate StorageDb::get_mut_connection() method just so we can use .transaction().
| // Even though this consumes `self`, the fact we use an Arc<> means | ||
| // we can't guarantee we can actually consume the inner DB - so do | ||
| // the best we can. | ||
| let shared: ThreadSafeStorageDb = match Arc::try_unwrap(self.db) { | ||
| let shared: ThreadSafeStorageDb = match Arc::try_unwrap(self.db.clone()) { |
There was a problem hiding this comment.
Hmmm, I don’t think this is going to work, though: self.db is already an owned Arc, so we know its reference count is at least 1. Cloning it will bump its reference count by 1, to at least 2, so Arc::try_unwrap will always fail.
(Newer versions of Rust also added Arc::into_inner, which I think we should switch to while we’re here. try_unwrap is for the case where you want to keep the Arc if it has > 1 reference; into_inner is when you don’t, which is what we’re doing here, and avoids a potential race condition. But both try_unwrap and into_inner need the Arc’s reference count to be 1 to succeed).
I think this close needs to stay a consuming (self) method. Was there a reason you had to change this one, or just for completeness?
This is so not ready for primetime . . . enter at your own risk.
Pull Request checklist
[ci full]to the PR title.Branch builds: add
[firefox-android: branch-name]to the PR title.