Bug 1597898 - Part 3: Update kvstore to use RKV in safe mode, r=nanj

Differential Revision: https://phabricator.services.mozilla.com/D54282
This commit is contained in:
Victor Porof 2020-07-24 13:53:02 +00:00
parent 05a29cb673
commit 2956e5494d
5 changed files with 30 additions and 125 deletions

View File

@ -7,7 +7,7 @@ use nserror::{
NS_ERROR_NULL_POINTER, NS_ERROR_UNEXPECTED,
};
use nsstring::nsCString;
use rkv::{migrate::MigrateError, StoreError};
use rkv::{MigrateError, StoreError};
use std::{io::Error as IoError, str::Utf8Error, string::FromUtf16Error, sync::PoisonError};
#[derive(Debug, Fail)]

View File

@ -8,10 +8,7 @@ extern crate crossbeam_utils;
extern crate cstr;
#[macro_use]
extern crate failure;
#[macro_use]
extern crate lazy_static;
extern crate libc;
extern crate lmdb;
extern crate log;
extern crate moz_task;
extern crate nserror;
@ -23,7 +20,6 @@ extern crate thin_vec;
extern crate xpcom;
mod error;
mod manager;
mod owned_value;
mod task;
@ -37,7 +33,8 @@ use moz_task::{
use nserror::{nsresult, NS_ERROR_FAILURE, NS_ERROR_NO_AGGREGATION, NS_OK};
use nsstring::{nsACString, nsCString};
use owned_value::{owned_to_variant, variant_to_owned};
use rkv::{OwnedValue, Rkv, SingleStore};
use rkv::backend::{SafeModeDatabase, SafeModeEnvironment};
use rkv::OwnedValue;
use std::{
ptr,
sync::{Arc, RwLock},
@ -57,6 +54,8 @@ use xpcom::{
nsIID, xpcom, xpcom_method, RefPtr,
};
type Rkv = rkv::Rkv<SafeModeEnvironment>;
type SingleStore = rkv::SingleStore<SafeModeDatabase>;
type KeyValuePairResult = Result<(String, OwnedValue), KeyValueError>;
#[no_mangle]

View File

@ -1,76 +0,0 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/. */
//! A manager of handles to Rkv environments that ensures that multiple calls
//! to `nsIKeyValueService.get_or_create()` with the same path reuse the same
//! environment handle.
//!
//! The Rkv crate itself provides this functionality, but it calls
//! `Path.canonicalize()` on the path, which crashes in Firefox on Android
//! because of https://bugzilla.mozilla.org/show_bug.cgi?id=1531887.
//!
//! Since kvstore consumers use canonical paths in practice, this manager
//! works around that bug by not doing so itself.
use error::KeyValueError;
use lmdb::Error as LmdbError;
use rkv::{migrate::Migrator, Rkv, StoreError};
use std::collections::hash_map::Entry;
use std::collections::HashMap;
use std::fs::copy;
use std::path::{Path, PathBuf};
use std::sync::{Arc, RwLock};
use tempfile::tempdir;
lazy_static! {
static ref MANAGER: RwLock<Manager> = RwLock::new(Manager::new());
}
pub(crate) struct Manager {
environments: HashMap<PathBuf, Arc<RwLock<Rkv>>>,
}
impl Manager {
fn new() -> Manager {
Manager {
environments: Default::default(),
}
}
pub(crate) fn singleton() -> &'static RwLock<Manager> {
&*MANAGER
}
/// Return the open env at `path`, or create it by calling `f`.
pub fn get_or_create<'p, F, P>(
&mut self,
path: P,
f: F,
) -> Result<Arc<RwLock<Rkv>>, KeyValueError>
where
F: Fn(&Path) -> Result<Rkv, StoreError>,
P: Into<&'p Path>,
{
let path = path.into();
Ok(match self.environments.entry(path.to_path_buf()) {
Entry::Occupied(e) => e.get().clone(),
Entry::Vacant(e) => {
let env = match f(e.key().as_path()) {
Ok(env) => env,
Err(StoreError::LmdbError(LmdbError::Invalid)) => {
let temp_env = tempdir()?;
let mut migrator = Migrator::new(path)?;
migrator.migrate(temp_env.path())?;
copy(temp_env.path().join("data.mdb"), path.join("data.mdb"))?;
copy(temp_env.path().join("lock.mdb"), path.join("lock.mdb"))?;
f(e.key().as_path())?
}
Err(err) => return Err(err.into()),
};
let k = Arc::new(RwLock::new(env));
e.insert(k).clone()
}
})
}
}

View File

@ -6,12 +6,12 @@ extern crate xpcom;
use crossbeam_utils::atomic::AtomicCell;
use error::KeyValueError;
use manager::Manager;
use moz_task::Task;
use nserror::{nsresult, NS_ERROR_FAILURE};
use nsstring::nsCString;
use owned_value::owned_to_variant;
use rkv::{OwnedValue, Rkv, SingleStore, StoreError, StoreOptions, Value};
use rkv::backend::{BackendInfo, SafeMode, SafeModeDatabase, SafeModeEnvironment};
use rkv::{Migrator, OwnedValue, StoreError, StoreOptions, Value};
use std::{
path::Path,
str,
@ -29,6 +29,10 @@ use KeyValueDatabase;
use KeyValueEnumerator;
use KeyValuePairResult;
type Manager = rkv::Manager<SafeModeEnvironment>;
type Rkv = rkv::Rkv<SafeModeEnvironment>;
type SingleStore = rkv::SingleStore<SafeModeDatabase>;
/// A macro to generate a done() implementation for a Task.
/// Takes one argument that specifies the type of the Task's callback function:
/// value: a callback function that takes a value
@ -98,14 +102,14 @@ const INCREMENTAL_RESIZE_THRESHOLD: usize = 52_428_800;
/// The incremental resize step (5 MB)
const INCREMENTAL_RESIZE_STEP: usize = 5_242_880;
/// The LMDB disk page size and mask.
/// The RKV disk page size and mask.
const PAGE_SIZE: usize = 4096;
const PAGE_SIZE_MASK: usize = 0b_1111_1111_1111;
/// Round the non-zero size to the multiple of page size greater or equal.
///
/// It does not handle the special cases such as size zero and overflow,
/// because even if that happens (extremely unlikely though), LMDB will
/// because even if that happens (extremely unlikely though), RKV will
/// ignore the new size if it's smaller than the current size.
///
/// E.g:
@ -190,11 +194,15 @@ impl Task for GetOrCreateTask {
self.result
.store(Some(|| -> Result<RkvStoreTuple, KeyValueError> {
let store;
let mut writer = Manager::singleton().write()?;
let rkv = writer.get_or_create(Path::new(str::from_utf8(&self.path)?), Rkv::new)?;
let mut manager = Manager::singleton().write()?;
// Note that path canonicalization is diabled to work around crashes on Fennec:
// https://bugzilla.mozilla.org/show_bug.cgi?id=1531887
let path = Path::new(str::from_utf8(&self.path)?);
let rkv = manager.get_or_create(path, Rkv::new::<SafeMode>)?;
Migrator::auto_migrate_lmdb_to_safe_mode(path, |builder| builder, rkv.read()?)?;
{
let env = rkv.read()?;
let load_ratio = env.load_ratio()?;
let load_ratio = env.load_ratio()?.unwrap_or(0.0);
if load_ratio > RESIZE_RATIO {
active_resize(&env)?;
}
@ -246,7 +254,7 @@ impl Task for PutTask {
let mut resized = false;
// Use a loop here in case we want to retry from a recoverable
// error such as `lmdb::Error::MapFull`.
// error such as `StoreError::MapFull`.
loop {
let mut writer = env.write()?;
@ -255,7 +263,7 @@ impl Task for PutTask {
// Only handle the first MapFull error via passive resizing.
// Propogate the subsequent MapFull error.
Err(StoreError::LmdbError(lmdb::Error::MapFull)) if !resized => {
Err(StoreError::MapFull) if !resized => {
// abort the failed transaction for resizing.
writer.abort();
@ -331,7 +339,7 @@ impl Task for WriteManyTask {
let mut resized = false;
// Use a loop here in case we want to retry from a recoverable
// error such as `lmdb::Error::MapFull`.
// error such as `StoreError::MapFull`.
'outer: loop {
let mut writer = env.write()?;
@ -345,7 +353,7 @@ impl Task for WriteManyTask {
// Only handle the first MapFull error via passive resizing.
// Propogate the subsequent MapFull error.
Err(StoreError::LmdbError(lmdb::Error::MapFull)) if !resized => {
Err(StoreError::MapFull) if !resized => {
// Abort the failed transaction for resizing.
writer.abort();
@ -365,10 +373,10 @@ impl Task for WriteManyTask {
match self.store.delete(&mut writer, key) {
Ok(_) => (),
// LMDB fails with an error if the key to delete wasn't found,
// RKV fails with an error if the key to delete wasn't found,
// and Rkv returns that error, but we ignore it, as we expect most
// of our consumers to want this behavior.
Err(StoreError::LmdbError(lmdb::Error::NotFound)) => (),
Err(StoreError::KeyValuePairNotFound) => (),
Err(err) => return Err(KeyValueError::StoreError(err)),
};
@ -528,10 +536,10 @@ impl Task for DeleteTask {
match self.store.delete(&mut writer, key) {
Ok(_) => (),
// LMDB fails with an error if the key to delete wasn't found,
// RKV fails with an error if the key to delete wasn't found,
// and Rkv returns that error, but we ignore it, as we expect most
// of our consumers to want this behavior.
Err(StoreError::LmdbError(lmdb::Error::NotFound)) => (),
Err(StoreError::KeyValuePairNotFound) => (),
Err(err) => return Err(KeyValueError::StoreError(err)),
};
@ -672,9 +680,8 @@ impl Task for EnumerateTask {
// Convert the key/value pair to owned.
.map(|result| match result {
Ok((key, val)) => match (key, val) {
(Ok(key), Some(val)) => Ok((key.to_owned(), OwnedValue::from(&val))),
(Ok(key), val) => Ok((key.to_owned(), OwnedValue::from(&val))),
(Err(err), _) => Err(err.into()),
(_, None) => Err(KeyValueError::UnexpectedValue),
},
Err(err) => Err(KeyValueError::StoreError(err)),
})

View File

@ -40,7 +40,7 @@ add_task(async function getOrCreate() {
);
await Assert.rejects(
KeyValueService.getOrCreate(nonexistentDir, "db"),
/DirectoryDoesNotExistError/
/UnsuitableEnvironmentPath/
);
// Test creating a database with a non-normalized but fully-qualified path.
@ -592,28 +592,3 @@ add_task(async function enumeration() {
await database.delete("string-key");
await database.delete("bool-key");
});
add_task(async function migration() {
const currentDir = await OS.File.getCurrentDirectory();
const databaseDir = await makeDatabaseDir("migration");
// We're testing migration from a different architecture to our own,
// so we choose the 32-bit database if we're a 64-bit build, and vice-versa.
const testEnvDir = Services.appinfo.is64Bit ? "test-env-32" : "test-env-64";
await OS.File.copy(
OS.Path.join(currentDir, "data", testEnvDir, "data.mdb"),
OS.Path.join(databaseDir, "data.mdb")
);
await OS.File.copy(
OS.Path.join(currentDir, "data", testEnvDir, "lock.mdb"),
OS.Path.join(databaseDir, "lock.mdb")
);
const database = await KeyValueService.getOrCreate(databaseDir, "db");
Assert.strictEqual(await database.get("int-key"), 1234);
Assert.strictEqual(await database.get("double-key"), 56.78);
Assert.strictEqual(await database.get("string-key"), "Héllo, wőrld!");
Assert.strictEqual(await database.get("bool-key"), true);
});