Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
3 tasks done
qryxip opened this issue Sep 25, 2024 · 1 comment · Fixed by #849
Closed
3 tasks done

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

qryxip opened this issue Sep 25, 2024 · 1 comment · Fixed by #849

Comments

@qryxip
Copy link
Member

qryxip commented Sep 25, 2024

内容

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. [project-vvm-async-api] "buffer"をRustの世界で保持し続ける #525CStringDropCheckerの設計のように、"new"したVoicevox…型についてwhiltelistを管理する
  2. [project-vvm-async-api] "buffer"をRustの世界で保持し続ける #525SliceOwnerの設計のように、オブジェクトの実体を完全に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の種類/ディストリ/バージョン

  • Windows
  • macOS
  • Linux

その他

@Hiroshiba
Copy link
Member

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

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

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

qryxip added a commit to qryxip/voicevox_core that referenced this issue Oct 5, 2024
C APIにおいて次の状況に対してセーフティネットを張り、パニックするように
する。

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

このPRは VOICEVOX#836 の解決**ではなく**、ドキュメントにも手を加えていない。とい
うのも`VoicevoxVoiceModelFile`には次のゲッターメソッドがあり、これらをカ
バーするのは現状のAPIの形だと不可能だからである。

* `voicevox_voice_model_file_id`
* `voicevox_voice_model_file_get_metas_json`
qryxip added a commit to qryxip/voicevox_core that referenced this issue Oct 5, 2024
C APIにおいて次の状況に対してセーフティネットを張り、パニックするように
する。

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

このPRは VOICEVOX#836 の解決**ではなく**、ドキュメントにも手を加えていない。とい
うのも`VoicevoxVoiceModelFile`には次のゲッターメソッドがあり、これらをカ
バーするのは現状のAPIの形だと不可能だからである。

* `voicevox_voice_model_file_id`
* `voicevox_voice_model_file_get_metas_json`
@qryxip qryxip closed this as completed in f2e6b60 Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants