Revert "Give up on busy DB after a while"
This reverts commit 115c4722f817abd904a017eecec2fc9eef4c9165.
Reason for revert: It turns out that some apparently-hung transactions
can eventually progress, after a longer period than the timeout used
here, so remove this behaviour.
Test: keystore2_tests
Bug: 319563050
Bug: 315165314
Bug: 346653802
Change-Id: Iabdff493c063d9afc31f34d47bf5e60202aa5700
diff --git a/keystore2/src/database.rs b/keystore2/src/database.rs
index 50cd3ba..a7f6a22 100644
--- a/keystore2/src/database.rs
+++ b/keystore2/src/database.rs
@@ -125,21 +125,6 @@
/// If the database returns a busy error code, retry after this interval.
const DB_BUSY_RETRY_INTERVAL: Duration = Duration::from_micros(500);
-/// If the database returns a busy error code, keep retrying for this long.
-const MAX_DB_BUSY_RETRY_PERIOD: Duration = Duration::from_secs(15);
-
-/// Check whether a database lock has timed out.
-fn check_lock_timeout(start: &std::time::Instant, timeout: Duration) -> Result<()> {
- if keystore2_flags::database_loop_timeout() {
- let elapsed = start.elapsed();
- if elapsed >= timeout {
- error!("Abandon locked DB after {elapsed:?}");
- return Err(&KsError::Rc(ResponseCode::BACKEND_BUSY))
- .context(ks_err!("Abandon locked DB after {elapsed:?}",));
- }
- }
- Ok(())
-}
impl_metadata!(
/// A set of metadata for key entries.
@@ -1397,18 +1382,6 @@
where
F: Fn(&Transaction) -> Result<(bool, T)>,
{
- self.with_transaction_timeout(behavior, MAX_DB_BUSY_RETRY_PERIOD, f)
- }
- fn with_transaction_timeout<T, F>(
- &mut self,
- behavior: TransactionBehavior,
- timeout: Duration,
- f: F,
- ) -> Result<T>
- where
- F: Fn(&Transaction) -> Result<(bool, T)>,
- {
- let start = std::time::Instant::now();
let name = behavior.name();
loop {
let result = self
@@ -1427,7 +1400,6 @@
Ok(result) => break Ok(result),
Err(e) => {
if Self::is_locked_error(&e) {
- check_lock_timeout(&start, timeout)?;
std::thread::sleep(DB_BUSY_RETRY_INTERVAL);
continue;
} else {
@@ -2156,7 +2128,6 @@
check_permission: impl Fn(&KeyDescriptor, Option<KeyPermSet>) -> Result<()>,
) -> Result<(KeyIdGuard, KeyEntry)> {
let _wp = wd::watch("KeystoreDB::load_key_entry");
- let start = std::time::Instant::now();
loop {
match self.load_key_entry_internal(
@@ -2169,7 +2140,6 @@
Ok(result) => break Ok(result),
Err(e) => {
if Self::is_locked_error(&e) {
- check_lock_timeout(&start, MAX_DB_BUSY_RETRY_PERIOD)?;
std::thread::sleep(DB_BUSY_RETRY_INTERVAL);
continue;
} else {
@@ -5523,81 +5493,4 @@
assert_eq!(third_sid_apps, vec![second_app_id]);
Ok(())
}
-
- #[test]
- fn test_key_id_guard_immediate() -> Result<()> {
- if !keystore2_flags::database_loop_timeout() {
- eprintln!("Skipping test as loop timeout flag disabled");
- return Ok(());
- }
- // Emit logging from test.
- android_logger::init_once(
- android_logger::Config::default()
- .with_tag("keystore_database_tests")
- .with_max_level(log::LevelFilter::Debug),
- );
-
- // Preparation: put a single entry into a test DB.
- let temp_dir = Arc::new(TempDir::new("key_id_guard_immediate")?);
- let temp_dir_clone_a = temp_dir.clone();
- let temp_dir_clone_b = temp_dir.clone();
- let mut db = KeystoreDB::new(temp_dir.path(), None)?;
- let key_id = make_test_key_entry(&mut db, Domain::APP, 1, TEST_ALIAS, None)?.0;
-
- let (a_sender, b_receiver) = std::sync::mpsc::channel();
- let (b_sender, a_receiver) = std::sync::mpsc::channel();
-
- // First thread starts an immediate transaction, then waits on a synchronization channel
- // before trying to get the `KeyIdGuard`.
- let handle_a = thread::spawn(move || {
- let temp_dir = temp_dir_clone_a;
- let mut db = KeystoreDB::new(temp_dir.path(), None).unwrap();
-
- // Make sure the other thread has initialized its database access before we lock it out.
- a_receiver.recv().unwrap();
-
- let _result =
- db.with_transaction_timeout(Immediate("TX_test"), Duration::from_secs(3), |_tx| {
- // Notify the other thread that we're inside the immediate transaction...
- a_sender.send(()).unwrap();
- // ...then wait to be sure that the other thread has the `KeyIdGuard` before
- // this thread also tries to get it.
- a_receiver.recv().unwrap();
-
- let _guard = KEY_ID_LOCK.get(key_id);
- Ok(()).no_gc()
- });
- });
-
- // Second thread gets the `KeyIdGuard`, then waits before trying to perform an immediate
- // transaction.
- let handle_b = thread::spawn(move || {
- let temp_dir = temp_dir_clone_b;
- let mut db = KeystoreDB::new(temp_dir.path(), None).unwrap();
- // Notify the other thread that we are initialized (so it can lock the immediate
- // transaction).
- b_sender.send(()).unwrap();
-
- let _guard = KEY_ID_LOCK.get(key_id);
- // Notify the other thread that we have the `KeyIdGuard`...
- b_sender.send(()).unwrap();
- // ...then wait to be sure that the other thread is in the immediate transaction before
- // this thread also tries to do one.
- b_receiver.recv().unwrap();
-
- let result =
- db.with_transaction_timeout(Immediate("TX_test"), Duration::from_secs(3), |_tx| {
- Ok(()).no_gc()
- });
- // Expect the attempt to get an immediate transaction to fail, and then this thread will
- // exit and release the `KeyIdGuard`, allowing the other thread to complete.
- assert!(result.is_err());
- check_result_is_error_containing_string(result, "BACKEND_BUSY");
- });
-
- let _ = handle_a.join();
- let _ = handle_b.join();
-
- Ok(())
- }
}