-
Notifications
You must be signed in to change notification settings - Fork 225
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
[WIP] Refactored webext-storage database close function #6424
base: main
Are you sure you want to change the base?
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. |
39c9321
to
9595b1d
Compare
9595b1d
to
2d0d734
Compare
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Thanks for cleaning up the no-longer-needed forget()
, too!
} | ||
} | ||
|
||
impl Deref for StorageDb { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.