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

ファイル保存全般をアトミックにする #2308

Merged
merged 5 commits into from
Oct 27, 2024

Conversation

tsunekazuomija
Copy link
Contributor

@tsunekazuomija tsunekazuomija commented Oct 22, 2024

ファイル保存全般をアトミックにする

@RikitoNoto さんが #2098 で作成してくださったファイル保存を
src/helpers/fileHelper.tsにヘルパー関数として切り出しました。
上記のファイルはレンダラープロセスからも参照されるため、
src/backend/electron/fileHelper.tsにヘルパー関数として切り出しました。

それに伴い、

  • src/backend/electron/electronConfig.tsElectronConfigManager.save
  • src/backend/electron/main.tsWRITE_FILE部分
  • src/backend/electron/manager/RuntimeInfoManager.tsexportFile 関数

のファイル保存処理をヘルパー関数で置き換えました。

それぞれ、

  • 設定/ツールバーのカスタマイズから設定を変更し、保存ボタンを押した時
  • プロジェクトを変更し、保存した時
  • 起動時のruntime-info.json生成時

にwriteFileSafelyのmoveFile呼び出しの前でelectronを停止したときにtmpファイルが存在しており、electronを再起動してファイルが正しく開ける/正しく起動できることを確認しました。

関連 Issue

ref #2103 #2098
close #2103

その他

src/backend/electron/main.tsWRITE_FILE部分を非同期関数に変えてしまったのですが、こちらで問題ないでしょうか?

@tsunekazuomija tsunekazuomija requested a review from a team as a code owner October 22, 2024 06:38
@tsunekazuomija tsunekazuomija requested review from Hiroshiba and removed request for a team October 22, 2024 06:38
@tsunekazuomija
Copy link
Contributor Author

ビルドに失敗してますね...
解決できそうか調べてみます!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fileHelper.tsはレンダラープロセスで使用されています。
そのためnodeAPI(fsmove-file)をインポートするとバンドルに失敗してビルドが失敗しているのではないかと思います。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

webアプリ全般に詳しくなく、見当がついていない状況だったので、レビュー大変助かります!

fileHelper.tsはレンダラープロセスで使用されています。
そのためnodeAPI(fsやmove-file)をインポートするとバンドルに失敗してビルドが失敗しているのではないかと思います。

なるほどです。であれば、追加するヘルパー関数はsrc/backend/electron以下に別途ファイルを作るなどして、メインプロセスからのみ使用するファイルに記載する必要がありそうですね。

メインプロセス、レンダラープロセスについて理解していない部分があるので、調べつつ修正してみます!

Copy link
Member

@Hiroshiba Hiroshiba Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

あっ!!! なるほど、すみませんでした!!!!

とりあえず一旦の解決策としては、electron用のディレクトリにヘルパー関数を置くのが良さそうだと思います!!
(と言おうとしたら既にそう提案されてますね!!! 非常に良いと思います!!!)

ちなみに取っ掛かりとしてelectronの仕組みをちょっとだけご説明すると。
electronはchromiumブラウザでGUIを表示するプロセス(レンダラー側)と、あとはそのGUIを起動したり、ブラウザではできないことをする用のメインプロセスに分かれていて、ファイル操作はメインプロセスじゃないとできないようになっている感じです!
なのでレンダラー側からファイル操作の処理をimportしたときに、そんなものがないのでエラーになる感じかなと・・・!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

お気になさらず…!むしろ、勉強になってよかったなと
electronについても教えていただき、ありがとうございます!

修正してみるので、お待ちいただければです!

@tsunekazuomija
Copy link
Contributor Author

修正してみました!
レビューをおねがいします

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!!

実装ありがとうございます、良さそう!!

ちょっとこっちでいくつか調整させていただこうと思います!
非同期なfs.promises.writeFileを使っていますが、場合によってはasyncを使わなくても実行できるコードの方が嬉しそうなので、同期実行する関数の方に変えさせていただこうと思います!

Comment on lines 9 to 12
const temp_path = `${path}.tmp`;
await fs.promises.writeFile(temp_path, data);

await moveFile(temp_path, path, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

あ、これ元コードで気づかなかったけどsnake_caseになってますね!
ちょっとついでにcamelCaseに直しちゃえると!!

Suggested change
const temp_path = `${path}.tmp`;
await fs.promises.writeFile(temp_path, data);
await moveFile(temp_path, path, {
const tempPath = `${path}.tmp`;
await fs.promises.writeFile(tempPath, data);
await moveFile(tempPath, path, {

Comment on lines 4 to 8
export async function writeFileSafely(
path: string,
data: string | NodeJS.ArrayBufferView,
) {
// ファイル書き込みに失敗したときに設定が消えないように、tempファイル書き込み後上書き移動する
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コメントはこう書いておくと、VSCodeで関数をマウスオーバーした時にドキュメントが表示されてちょっと便利だったりします!

/** ファイル書き込みに失敗したときに設定が消えないように、tempファイル書き込み後上書き移動する */

Suggested change
export async function writeFileSafely(
path: string,
data: string | NodeJS.ArrayBufferView,
) {
// ファイル書き込みに失敗したときに設定が消えないように、tempファイル書き込み後上書き移動する
export async function writeFileSafely(
path: string,
data: string | NodeJS.ArrayBufferView,
) {

@Hiroshiba
Copy link
Member

あっとすみません!
メンテナからの書き込み権限がなさそうなので、ちょっと変更いただけると!!

export async function writeFileSafely(の、asyncをなくせるようにできると嬉しい感じです!
たぶんmoveFilemoveFileSyncfs.promises.writeFilefs.writeFileSyncにするとできる・・・はず・・・!

@tsunekazuomija
Copy link
Contributor Author

ああ、こちらリポジトリの設定でお手数をお掛けしてすみません!
フィードバックもありがとうございます!修正してみます

@tsunekazuomija
Copy link
Contributor Author

  • 同期関数にする
  • 変数をcamelCaseにする
  • 関数のコメントを /** */で記述する

の3点を修正してみました!

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!!

調整ありがとうございました!!!
プルリクエストもありがとうございました!

もしよかったらまたプルリクエストお待ちしています!!
分からないことがあったらお気軽に聞いてください!

@Hiroshiba Hiroshiba merged commit cb58a10 into VOICEVOX:main Oct 27, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ファイル保存全般をアトミック操作にする
3 participants