Adding shutdown support to sql-support #4816
Conversation
| /// - Calls `shutdown::register_interrupt_handle()` on creation | ||
| /// | ||
| /// See `PlacesDb::begin_interrupt_scope()` and `PlacesApi::new_connection()` for an example of | ||
| /// how this works. |
There was a problem hiding this comment.
It's kind of weird that this module is inside the sql-support crate, but it kind of needs to be because of how it integrates with SqlInterruptScope and SqlInterruptHandler. I think we should consider consider moving both shutdown.rs and interrupt.rs to the interrupt-support crate.
Codecov Report
@@ Coverage Diff @@
## main #4816 +/- ##
=======================================
Coverage 81.49% 81.49%
=======================================
Files 49 49
Lines 5653 5653
=======================================
Hits 4607 4607
Misses 1046 1046 Continue to review full report at Codecov.
|
When the user wants to shutdown the application, we should: - Interrupt all current `SqlInterruptScope`s - Interrupt all future `SqlInterruptScope`s when they're created. The nice thing about this approach is that it didn't require invasive changes in places to support it. The main new requirement was we need to have a way to get a `Weak<AsRef<SqlInterruptHandler>>` for each database. In order to support that, I needed to: - For the read/write and read-only connections: have `PlacesConnection` store an `SqlInterruptHandler` and implement `AsRef`. - For the sync connection: Added struct that wraps the `Mutex<PlacesDb>` and also stores a `SqlInterruptHandler` and implements `AsRef`. Updated `places-utils` so that ctrl-c starts shutdown mode.
bendk
force-pushed
the
simple-shutdown
branch
from
c6813af
to
91d0bab
Compare
|
I think this is a step forward so I'm planning on merging once the checks pass. However, there are still several steps left to fully implement #1684. I left a comment in that ticket outlining the remaining work. |
I think 3rd time's the charm on this one. This PR is similar to #4805, but much simpler. It implements a shutdown system that hooks into the existing
SqlInterruptScopeandSqlInterruptHandlerfunctionality, which means that the changes are a lot less invasive. We end up with each DB being able to create anSqlInterruptScope, but with a global shutdown mode, that interrupts all of those scopes. So we end up with being able to both globally shutdown everything and cause all operations to be interrupted and also locally shutdown a single operation (e.g. autocomplete).You can test this with the
places-utilsscript:- Execute
cargo run --example places-utils sync --nsyncs 2 --wait 1000Pull Request checklist
[ci full]to the PR title.