dfacto-lab / serilog-sinks-file

Serilog file sinks duplicated for file rolling
Apache License 2.0
14 stars 10 forks source link

Infinite loop when configured for size-only file rolling #18

Open palpha opened 2 years ago

palpha commented 2 years ago

Repro: have log file that is larger than size limit; configure to roll on size limit, not on interval and not on process run; create logger. Result: infinite loop, because RollingFileSink.MustRoll returns false when interval is Infinite.

I fixed this to work as expected by changing MustRoll to return _rollOnFileSizeLimit if !currentCheckpoint.HasValue, but that's obviously a duct tape solution. I don't know enough about the project code to submit a reasonable PR, but I hope my tiny investigation can help someone more knowledgeable to fix this issue without having to spend too much energy.

bartelink commented 2 years ago

Sorry for sticking my oar in but ... does this repro on the OG Serilog.sinks.file? If it can, then it could be fixed there and then merged into this (there will also be more eyes there, but obviously that only makes sense if it fails there too)?

palpha commented 2 years ago

Ah! Of course, I should have checked that first. Will do first thing Monday. The immediately suspect code is not present there, as far as I can tell, but it might still be an issue.

numito commented 2 years ago

Hello,

I will have a look.

Numa Schmeder

Le 4 févr. 2022 à 21:10, Niklas Bergius @.***> a écrit :

 Ah! Of course, I should have checked that first. Will do first thing Monday. The immediately suspect code is not present there, as far as I can tell, but it might still be an issue.

— Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you are subscribed to this thread.

palpha commented 2 years ago

does this repro on the OG Serilog.sinks.file?

The answer is no, I'm sorry to report.

numito commented 2 years ago

This is strange, I have a version with size limit instead of interval configured in production without issue. I have to check there might have been a change in the meantime.

palpha commented 2 years ago

I realize now that my repro is kind of artificial – you would rarely end up starting the application with a log file that is larger than the limit, unless it was reconfigured and rebooted. I will check if normal operation, where the log file is empty and reaches the limit while the application is running, works as intended with rollOnEachProcessRun: false, rollOnFileSizeLimit: true and RollingInterval.Infinite. If so, this is not a big problem.

palpha commented 2 years ago

Have just confirmed that it simply stops logging when reaching the size limit. This is my configuration:

fileSizeLimitBytes: 14000,
rollOnFileSizeLimit: true,
shared: false,
flushToDiskInterval: TimeSpan.FromSeconds( 1 ),
retainedFileCountLimit: 100,
buffered: true,
rollOnEachProcessRun: false,
preserveLogFilename: true,
persistentFileRollingInterval: PersistentFileRollingInterval.Infinite