VOICEVOX / voicevox_core

無料で使える中品質なテキスト読み上げソフトウェア、VOICEVOXのコア
https://voicevox.hiroshiba.jp/
MIT License
871 stars 117 forks source link

C APIの"delete"を安全なAPIにする #836

Closed qryxip closed 1 month ago

qryxip commented 2 months ago

内容

C APIにてVoicevoxSynthesizerなどの#[repr(Rust)]な型について、"delete"を「安全」なAPIにします。安全とは、プロセスをクラッシュさせることになるにしてもRustのUBを起こさないことです。

Pros 良くなる点

Cons 悪くなる点

実現方法

(以下の説明は、どちらかというと私(qryxip)の中での整理という点が強いです)

安全と呼べるのに満たすべき性質は次のようになると思います。

  1. オブジェクトが他スレッドでアクセスされている最中に"delete"してもUBではない
  2. "delete"後に他の通常のメソッド関数の利用を試みてもUBではない
  3. "delete"後に"delete"を試みてもUBではない

1.だけ満たすのは #832 のようにRwLock中心に設計すればいいのですが、2.と3.が厄介です。Voicevox…型を単にRwLock駆動にしただけだと、2.と3.でBox<_>にアクセスしていいかどうか判断できません。対処としては次のようになると思います。まあどっちも面倒そう。

  1. 525 のCStringDropCheckerの設計のように、"new"したVoicevox…型についてwhiltelistを管理する

  2. 525 のSliceOwnerの設計のように、オブジェクトの実体を完全にRust側で管理する

方針1.はちゃんとやろうとするとロックのタイミングが面倒なことになりそう。

方針2.については、まずVoicevox…型の定義をこうしておき、

// C APIとしてはopaqueなstructにする
#[repr(Rust)]
struct VoicevoxSynthesizer {
    id: u32,
}

Voicevox…型の実体はこういう風に必要に応じて伸びるリストで管理し、プロセスの終わりまで決してデアロケートされないようにします。

// 伸びることはあっても縮むことはない
[{id: 0}, {id: 1}, {id: 2}, {id: 3}, {id: 4}, …]

そしてRustのオブジェクト本体は次のように管理するようにします。

crossbeam_skiplist::SkipMap< // https://docs.rs/crate/crossbeam-skiplist
    u32, // id
    voicevox_core::blocking::Synthesizer, // Rust APIの"Synthesizer"
>

こうすればVoicevoxSynthesizerを"delete"してもRust APIのSynthesizerはしっかりとデストラクトされ、VoicevoxSynthesizer自体もデストラクトされたように振る舞い、しかし本当はデストラクトされていないのでUBを避けたフールプルーフが可能になる、といったことが実現できます。個人的にはこの方針2.を考えています。

VOICEVOXのバージョン

OSの種類/ディストリ/バージョン

その他

Hiroshiba commented 1 month ago

以前にも同じような感じの実装をしましたね! それがissueに書いてあるSliceOwnerとかかな。

確かに実装してあげると良さそうですが、少なくともドキュメントでUBだと案内されていればOKで、ちゃんと実装する優先度は比較的低めだと感じています。 たしか以前のは結構起こりやすい経路だった記憶。今回はdelete後での利用でUBとのことなので、しっかり書かれたコードではちょっと起こりにくそうだなと思ったので。

とはいえ起こり得る話ではあるので、実装にネガティブという感じではないです!