From c156ca4c76eab914d931a9f4cca60d8736a58c44 Mon Sep 17 00:00:00 2001 From: Ryo Yamashita Date: Fri, 25 Oct 2024 09:29:11 +0900 Subject: [PATCH] fix: fix up #849 (#862) Fix up #849. The main purpose is fixing the following bug using `boxcar`. https://github.com/VOICEVOX/voicevox_core/pull/849#discussion_r1814221605 Refs: #849 --- Cargo.lock | 9 ++- Cargo.toml | 1 + crates/voicevox_core_c_api/Cargo.toml | 1 + crates/voicevox_core_c_api/src/c_impls.rs | 17 +++-- crates/voicevox_core_c_api/src/object.rs | 75 +++++++++++++++-------- 5 files changed, 70 insertions(+), 33 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 335b33d38..dfb585fb2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -429,6 +429,12 @@ dependencies = [ "piper", ] +[[package]] +name = "boxcar" +version = "0.2.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fba19c552ee63cb6646b75e1166d1bdb8a6d34a6d19e319dec88c8adadff2db3" + [[package]] name = "bstr" version = "1.2.0" @@ -1963,7 +1969,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0c2a198fb6b0eada2a8df47933734e6d35d350665a33a3593d7164fa52c75c19" dependencies = [ "cfg-if", - "windows-targets 0.48.5", + "windows-targets 0.52.6", ] [[package]] @@ -4352,6 +4358,7 @@ dependencies = [ "anstyle-query", "anyhow", "assert_cmd", + "boxcar", "camino", "chrono", "clap", diff --git a/Cargo.toml b/Cargo.toml index 610affaa8..e3461156c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -14,6 +14,7 @@ async_zip = "=0.0.16" bindgen = "0.69.4" binstall-tar = "0.4.42" blocking = "1.6.1" +boxcar = "0.2.6" bytes = "1.7.2" camino = "1.1.9" cargo_metadata = "0.18.1" diff --git a/crates/voicevox_core_c_api/Cargo.toml b/crates/voicevox_core_c_api/Cargo.toml index bec06ef7c..c9600c7a6 100644 --- a/crates/voicevox_core_c_api/Cargo.toml +++ b/crates/voicevox_core_c_api/Cargo.toml @@ -20,6 +20,7 @@ link-onnxruntime = ["voicevox_core/link-onnxruntime"] [dependencies] anstream = { workspace = true, default-features = false, features = ["auto"] } anstyle-query.workspace = true +boxcar.workspace = true camino.workspace = true chrono = { workspace = true, default-features = false, features = ["clock"] } colorchoice.workspace = true diff --git a/crates/voicevox_core_c_api/src/c_impls.rs b/crates/voicevox_core_c_api/src/c_impls.rs index c657d0908..c83d9b3b0 100644 --- a/crates/voicevox_core_c_api/src/c_impls.rs +++ b/crates/voicevox_core_c_api/src/c_impls.rs @@ -1,6 +1,7 @@ use std::{ - collections::HashMap, + collections::{HashMap, HashSet}, ffi::CString, + num::NonZero, path::Path, ptr::NonNull, sync::{Arc, LazyLock}, @@ -145,17 +146,23 @@ fn metas_to_json(metas: &[SpeakerMeta]) -> CString { impl CApiObject for H { type RustApiObject = B; - fn heads() -> &'static std::sync::Mutex> { - static HEADS: std::sync::Mutex> = std::sync::Mutex::new(vec![]); + fn known_addrs() -> &'static std::sync::Mutex>> { + static KNOWN_ADDRS: LazyLock>>> = + LazyLock::new(Default::default); + &KNOWN_ADDRS + } + + fn heads() -> &'static boxcar::Vec { + static HEADS: boxcar::Vec = boxcar::Vec::new(); &HEADS } fn bodies() -> &'static std::sync::Mutex< - HashMap>>>, + HashMap, Arc>>>, > { #[expect(clippy::type_complexity, reason = "`CApiObject::bodies`と同様")] static BODIES: LazyLock< - std::sync::Mutex>>>>, + std::sync::Mutex, Arc>>>>, > = LazyLock::new(Default::default); &BODIES diff --git a/crates/voicevox_core_c_api/src/object.rs b/crates/voicevox_core_c_api/src/object.rs index a3aea19f1..ce5298a9a 100644 --- a/crates/voicevox_core_c_api/src/object.rs +++ b/crates/voicevox_core_c_api/src/object.rs @@ -1,8 +1,9 @@ use std::{ any, - collections::HashMap, + collections::{HashMap, HashSet}, fmt::{Debug, Display}, mem, + num::NonZero, ops::{Deref, DerefMut}, ptr::NonNull, sync::Arc, @@ -11,6 +12,9 @@ use std::{ use easy_ext::ext; use tracing::warn; +// FIXME: 次のような状況に備え、`new`をいっぱい行うテストを書く +// https://github.com/VOICEVOX/voicevox_core/pull/849#discussion_r1814221605 + /// プロセスの終わりまでデストラクトされない、ユーザーにオブジェクトとして貸し出す1-bit長の構造体。 /// /// インスタンスは次のような形。 @@ -31,8 +35,10 @@ use tracing::warn; pub(crate) trait CApiObject: Default + Debug + 'static { type RustApiObject: 'static; - // 書き込み操作としては`push`のみ - fn heads() -> &'static std::sync::Mutex>; + // 行う可変操作は`insert`のみ + fn known_addrs() -> &'static std::sync::Mutex>>; + + fn heads() -> &'static boxcar::Vec; #[expect( clippy::type_complexity, @@ -40,7 +46,7 @@ pub(crate) trait CApiObject: Default + Debug + 'static { )] fn bodies() -> &'static std::sync::Mutex< HashMap< - usize, // `heads`の要素へのポインタのアドレス + NonZero, // `heads`の要素へのポインタのアドレス Arc< parking_lot::RwLock< Option, // `RwLock`をdropする直前まで`Some` @@ -53,71 +59,79 @@ pub(crate) trait CApiObject: Default + Debug + 'static { assert!(mem::size_of::() > 0); let this = { - let mut heads = Self::lock_heads(); - heads.push(Default::default()); - NonNull::from(heads.last().expect("just pushed")) + let i = Self::heads().push(Default::default()); + NonNull::from(&Self::heads()[i]) }; + Self::lock_known_addrs().insert(this.addr_without_provenance()); let body = parking_lot::RwLock::new(body.into()).into(); - Self::lock_bodies().insert(this.as_ptr() as _, body); + Self::lock_bodies().insert(this.addr_without_provenance(), body); this } } #[ext(CApiObjectPtrExt)] impl *const T { + // ユーザーから渡されたポインタである`self`は、次のうちどれかに分類される。 + // + // 1. `known_addrs`に含まれない ⇨ 知らないどこかのダングリングポインタか何か。あるいはnull + // 2. `known_addrs`に含まれるが、`bodies`に含まれない → "delete"済み + // 3. `known_addrs`も`bodies`にも含まれる → 1.でも2.でもなく、有効 + /// # Panics /// /// 同じ対象に対して`drop_body`を呼んでいるとパニックする。 pub(crate) fn body(self) -> impl Deref { - self.validate(); + let this = self.validate(); let body = T::lock_bodies() - .get(&(self as _)) - .unwrap_or_else(|| self.panic_for_deleted()) + .get(&this.addr_without_provenance()) + .unwrap_or_else(|| this.panic_for_deleted()) .read_arc(); voicevox_core::__internal::interop::raii::try_map_guard(body, |body| { body.as_ref().ok_or(()) }) - .unwrap_or_else(|()| self.panic_for_deleted()) + .unwrap_or_else(|()| this.panic_for_deleted()) } /// # Panics /// /// 同じ対象に対してこの関数を二度呼ぶとパニックする。 pub(crate) fn drop_body(self) { - self.validate(); + let this = self.validate(); let body = T::lock_bodies() - .remove(&(self as _)) - .unwrap_or_else(|| self.panic_for_deleted()); + .remove(&this.addr_without_provenance()) + .unwrap_or_else(|| this.panic_for_deleted()); drop( body.try_write_arc() .unwrap_or_else(|| { warn!( "{this} is still in use. Waiting before closing", - this = self.display(), + this = this.display(), ); body.write_arc() }) .take() - .unwrap_or_else(|| self.panic_for_deleted()), + .unwrap_or_else(|| this.panic_for_deleted()), ); } } #[ext] impl *const T { - fn validate(self) { - if self.is_null() { - panic!("the argument must not be null"); - } - if !T::lock_heads().as_ptr_range().contains(&self) { + fn validate(self) -> NonNull { + let this = NonNull::new(self as *mut T).expect("the argument must not be null"); + if !T::lock_known_addrs().contains(&this.addr_without_provenance()) { panic!("{self:018p} does not seem to be valid object"); } + this } +} +#[ext] +impl NonNull { fn display(self) -> impl Display { let type_name = any::type_name::() .split("::") @@ -131,15 +145,22 @@ impl *const T { } } +#[ext] +impl NonNull { + fn addr_without_provenance(self) -> NonZero { + NonZero::new(self.as_ptr() as _).expect("this is from `NonNull`") + } +} + #[ext] impl T { - fn lock_heads() -> impl DerefMut> { - Self::heads().lock().unwrap_or_else(|e| panic!("{e}")) + fn lock_known_addrs() -> impl DerefMut>> { + Self::known_addrs().lock().unwrap_or_else(|e| panic!("{e}")) } - fn lock_bodies( - ) -> impl DerefMut>>>> - { + fn lock_bodies() -> impl DerefMut< + Target = HashMap, Arc>>>, + > { Self::bodies().lock().unwrap_or_else(|e| panic!("{e}")) } }