Skip to content

Commit

Permalink
fix: fix up #849 (#862)
Browse files Browse the repository at this point in the history
Fix up #849.

The main purpose is fixing the following bug using `boxcar`.

#849 (comment)

Refs: #849
  • Loading branch information
qryxip authored Oct 25, 2024
1 parent 23c2a62 commit c156ca4
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 33 deletions.
9 changes: 8 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
1 change: 1 addition & 0 deletions crates/voicevox_core_c_api/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 12 additions & 5 deletions crates/voicevox_core_c_api/src/c_impls.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::{
collections::HashMap,
collections::{HashMap, HashSet},
ffi::CString,
num::NonZero,
path::Path,
ptr::NonNull,
sync::{Arc, LazyLock},
Expand Down Expand Up @@ -145,17 +146,23 @@ fn metas_to_json(metas: &[SpeakerMeta]) -> CString {
impl CApiObject for H {
type RustApiObject = B;

fn heads() -> &'static std::sync::Mutex<Vec<Self>> {
static HEADS: std::sync::Mutex<Vec<H>> = std::sync::Mutex::new(vec![]);
fn known_addrs() -> &'static std::sync::Mutex<HashSet<NonZero<usize>>> {
static KNOWN_ADDRS: LazyLock<std::sync::Mutex<HashSet<NonZero<usize>>>> =
LazyLock::new(Default::default);
&KNOWN_ADDRS
}

fn heads() -> &'static boxcar::Vec<Self> {
static HEADS: boxcar::Vec<H> = boxcar::Vec::new();
&HEADS
}

fn bodies() -> &'static std::sync::Mutex<
HashMap<usize, Arc<parking_lot::RwLock<Option<Self::RustApiObject>>>>,
HashMap<NonZero<usize>, Arc<parking_lot::RwLock<Option<Self::RustApiObject>>>>,
> {
#[expect(clippy::type_complexity, reason = "`CApiObject::bodies`と同様")]
static BODIES: LazyLock<
std::sync::Mutex<HashMap<usize, Arc<parking_lot::RwLock<Option<B>>>>>,
std::sync::Mutex<HashMap<NonZero<usize>, Arc<parking_lot::RwLock<Option<B>>>>>,
> = LazyLock::new(Default::default);

&BODIES
Expand Down
75 changes: 48 additions & 27 deletions crates/voicevox_core_c_api/src/object.rs
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -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長の構造体。
///
/// インスタンスは次のような形。
Expand All @@ -31,16 +35,18 @@ use tracing::warn;
pub(crate) trait CApiObject: Default + Debug + 'static {
type RustApiObject: 'static;

// 書き込み操作としては`push`のみ
fn heads() -> &'static std::sync::Mutex<Vec<Self>>;
// 行う可変操作は`insert`のみ
fn known_addrs() -> &'static std::sync::Mutex<HashSet<NonZero<usize>>>;

fn heads() -> &'static boxcar::Vec<Self>;

#[expect(
clippy::type_complexity,
reason = "型を分離するとかえって可読性を失う。その代わりコメントを入れている"
)]
fn bodies() -> &'static std::sync::Mutex<
HashMap<
usize, // `heads`の要素へのポインタのアドレス
NonZero<usize>, // `heads`の要素へのポインタのアドレス
Arc<
parking_lot::RwLock<
Option<Self::RustApiObject>, // `RwLock`をdropする直前まで`Some`
Expand All @@ -53,71 +59,79 @@ pub(crate) trait CApiObject: Default + Debug + 'static {
assert!(mem::size_of::<Self>() > 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<T: CApiObject> *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<Target = T::RustApiObject> {
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<T: CApiObject> *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<T> {
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<T: CApiObject> NonNull<T> {
fn display(self) -> impl Display {
let type_name = any::type_name::<T>()
.split("::")
Expand All @@ -131,15 +145,22 @@ impl<T: CApiObject> *const T {
}
}

#[ext]
impl<T> NonNull<T> {
fn addr_without_provenance(self) -> NonZero<usize> {
NonZero::new(self.as_ptr() as _).expect("this is from `NonNull`")
}
}

#[ext]
impl<T: CApiObject> T {
fn lock_heads() -> impl DerefMut<Target = Vec<Self>> {
Self::heads().lock().unwrap_or_else(|e| panic!("{e}"))
fn lock_known_addrs() -> impl DerefMut<Target = HashSet<NonZero<usize>>> {
Self::known_addrs().lock().unwrap_or_else(|e| panic!("{e}"))
}

fn lock_bodies(
) -> impl DerefMut<Target = HashMap<usize, Arc<parking_lot::RwLock<Option<Self::RustApiObject>>>>>
{
fn lock_bodies() -> impl DerefMut<
Target = HashMap<NonZero<usize>, Arc<parking_lot::RwLock<Option<Self::RustApiObject>>>>,
> {
Self::bodies().lock().unwrap_or_else(|e| panic!("{e}"))
}
}

0 comments on commit c156ca4

Please sign in to comment.