search

cocoa-mhlw / cocoa

Mozilla Public License 2.0
991 stars 112 forks source link

ログ出力時に毎回ファイルを開いている #38

Open Takym opened 3 years ago

Takym commented 3 years ago

ログ出力時に以下のコードが呼び出され、毎回ログファイルを開いています。 https://github.com/cocoa-mhlw/cocoa/blob/d136940cccd7f096b0fc8f34172d8a9c293b2f8c/Covid19Radar/Covid19Radar/Services/Logs/LoggerService.cs#L139 毎回ファイルを開くのは無駄な処理ではないでしょうか？

参考

Internal Tracking ID: NFR 2450

keiji commented 3 years ago

ぼくも前に気になって調べたことがあるんですが、そのときは「ファイルを閉じるタイミングをどうしよう」となってしまいました。

IDisposableを実装しておけばIoCコンテナ（Dryloc）がコンテナ廃棄のタイミングでDisposeを実行してくれるんですね。

Takym commented 3 years ago

乱暴なやり方ですが、参考のコード（LogReaderWriter）では AutoFlush を true に設定して、ストリームを明示的に閉じない様にしています。アプリ終了時に OS が自動的にファイルハンドルを破棄してくれる筈です。

keiji commented 3 years ago

今、簡易的にIDisposableを実装してDiposeがかかるかやってみたんですが、Activityの終了時にコンテナが破棄されないのは当然として、Applicationの終了時にもDisposeが呼ばれている形跡がないですね。

プロセスが終了すれば関連のファイルハンドルは解放されるというのはわかるのですが若干の気持ち悪さがあります…… 🤔

Takym commented 3 years ago

ICloseApplication の closeApplication で破棄する事を考えましたが、終了時に必ず呼び出される訳ではないみたいですね。

keiji commented 3 years ago

Androidの場合、明示的にcloseしなくてもApplicationが破棄されるタイミングでクリーンアップするのに任せるのでOKですね。 （そういえば、RoomはApplicationで開くけど、閉じたことはない）

Android made a deliberate design decision that is can seem surprising, to just give up on the whole idea of applications cleanly exiting and instead let the kernel clean up their resources. After all, the kernel needs to be able to do this anyway. Given that design, keeping anything open for the entire duration of a process's life and never closing it is simply not a leak. It will be cleaned up when the process is cleaned up.

https://groups.google.com/g/android-developers/c/nopkaw4UZ9U/m/cPfPL3uW7nQJ

iOSはどうだろう

Takym commented 3 years ago

終了時の処理を記述する方法が見つかりました。

Android の場合は、MainActivity クラスで OnDestroy を上書きする事で終了イベントを記述できるみたいです。

/* 前略 */
public class MainActivity : global::Xamarin.Forms.Platform.Android.FormsAppCompatActivity
{
    /* 中略 */

    protected override void OnDestroy()
    {
        base.OnDestroy();
        // ここに終了時に実行すべき処理を記述する
    }

    /* 中略 */
}
/* 後略 */

iOS の場合は、AppDelegate クラスで WillTerminate を上書きする事で終了イベントを記述できるみたいです。

/* 前略 */
public partial class AppDelegate : global::Xamarin.Forms.Platform.iOS.FormsApplicationDelegate
{
    /* 中略 */

    public override void WillTerminate(UIApplication uiApplication)
    {
        base.WillTerminate(uiApplication);
        // ここに終了時に実行すべき処理を記述する
    }

    /* 中略 */
}
/* 後略 */

ここでログファイルのストリームを破棄する事ができそうです。

参考

ghost commented 3 years ago

Android の Activity#onDestroy は呼ばれる保証はありません

https://teratail.com/questions/117292#reply-188478

あと、そもそもバッテリー切れとかもあるので、もし自前の実装を続けるのであれば、ファイルのクローズはともかく、最低限 Flush は適当なタイミングでやってお祈りする感じになるんじゃないかと思います。

ちゃんとしたログ出力ライブラリを使ったほうがいいし、せめてそういう実績のあるコードをコピーしてくる感じがいいかもですね。。

https://github.com/NLog/NLog/blob/fbf4d179c421f6ce319bd5f9ec6cfeefc0ef3b0d/src/NLog/Internal/FileAppenders/MutexMultiProcessFileAppender.cs

keiji commented 3 years ago

Android の Activity#onDestroy は呼ばれる保証はありません

そうですね。 あとはAndroidではLoggerServiceの生存期間はMainActivityより長いMainApplicationと同じになるので、ActivityでdisposeしてしまうとApplication側で再度開かないといけない（開けるようにしないといけない）＋いつ閉じる？　という課題がありますね。

基本的にはApplicationが停止するときにファイルハンドルも解放されるから良い。と言う方向でいいと思います（iOSの挙動もおそらくそうなんでしょうが、確証を持つためにドキュメントを紐解きたい）。

ちゃんとしたログライブラリを使うのはよりよい選択肢と考えますが、このPR Issueの主題である「ログ出力時に毎回ファイルを開いている」への対応としては踏むべきステップが大きいので、そこは別Issueにした方がいいかなと思います。

UPDATE PRじゃなくてIssueでしたね。失礼しました。

Takym commented 3 years ago

すみません。まだ PR は作成していません。

Takym commented 3 years ago

返信ありがとう御座います。ログファイルの名前が変更された時（日付が変わった時等）はファイルを開き直す処理を入れれば良いと考えています。また、ログファイル提出時に正しく出力処理が行える様に、共有オプションで読み取りを許可すれば良いと考えています。'rotate' につきましては、利用中のログファイルは削除されないという認識です。

Takym commented 3 years ago

プルリクエストを作成しました。

keiji commented 3 years ago

LINQを使わず高速化する提案を記録しておく。

https://github.com/cocoa-mhlw/cocoa/pull/67#discussion_r630264389