Fix issue with force-reopen after 30 minutes #337
Conversation
|
@nblumhardt for review, thanks :) |
|
I also found a bug during testing with the On Windows this lead to the file not being accessible for the follow-up try while on Ubuntu it just worked fine. This will ensure it works reliably on both. |
This comment was marked as spam.
This comment was marked as spam.
| @@ -0,0 +1,36 @@ | |||
| using System.Text; | |||
|
|
|||
| namespace Serilog.Sinks.File.Tests.Support; | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
This makes no sense. Reported the user as spam bot to avoid further interaction.
There was a problem hiding this comment.
Thanks @Falco20019! Looks great 👍 Just had one minor comment.
| if (_currentFile == null) | ||
| { | ||
| SelfLog.WriteLine("Log event {0} was lost since it was not possible to open the file or create a new one.", logEvent.RenderMessage()); | ||
| } |
There was a problem hiding this comment.
Not sure about doing this under the lock, since SelfLog may be writing somewhere slow. Might be safer to drop this for now, and reconsider separately.
There was a problem hiding this comment.
I can do that, I just had issues already that stuff was lost and we never found out why. But will remove it to progress with the PR for now. This should really never happen on regular use-cases and therefore should be neglectable and if the file was locked, we also use the SelfLog, so it was the nearest thing to consider :)
I will just comment it out so that it's easy to add in the future.
|
Thanks @Falco20019 😎 |
|
@nblumhardt Can you please have this release as |
Fixes #334
I also added a check to
Emitthat writes toSelfLogif the log was dropped. This could have happend also before if the file was locked 3 times or any other exception was thrown due to checking for_currentFile?.EmitOrOverflow(logEvent) == falseand not_currentFile?.EmitOrOverflow(logEvent) != truebut it was hard to find out that it got dropped. But since the code seems to do this intentional to avoid blocking and force-retrying after 30 minutes (or on next checkpoint), that should be correct.I encapsulated everything into a
try-finallyblock to ensure that this happens on ANY exception. The old code was also waiting until the next checkpoint. So if that was daily, it would drop ALL logs for the rest of the day. With my adjustment (to checking if 30 minutes is less than the next checkpoint) it will also cover the case that no rolling was setup OR that it would be longer than 30 minutes.