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

Ability to delete log files and recreate them immediately #144

Closed
wants to merge 1 commit into from

Conversation

thiagosgarcia
Copy link
Contributor

As discussed in #96 and #128, this gives the ability to delete files when is created by a SharedFileSink and recreates the rolling file when deleted.
Please, note that this is still a initial version for the solution, I'm going to implement more tests and eliminate some duplication.

I'd like to start this discussion in order gather some insights from the project maintainers and address some questions I have. I'm gonna give some more thoughts on this one as soon as I can.

Allow Log File to be Deleted at Runtime #96

I was able to make this work by just adding FileShare.Delete at both FileStream instances. This would make all shared files 'deleteable'.

About that:

  • Is ok to make all shared files 'deleteable'?; or
  • Should I make another configuration and only enable deletion capability when is configured by the user? (eg.: canDeleteFiles: true)

Recreate current rolling file immediately to log on file deleting #128

This was a little more tricky, but I was able to make this work using SharedFileSink.OSMutex

Questions I have:

  • How do I reproduce/run SharedFileSink.AtomicAppend? I wasn't able to run this class in my tests or by debugging.
  • Should we be worried about the performance of File.Exists() at SharedFileSink.OSMutex.cs line 85? Do you guys have a better idea to make this check? This was actually how I managed to make the file be re-created right after deletion.

@@ -72,7 +72,7 @@ public SharedFileSink(string path, ITextFormatter textFormatter, long? fileSizeL
path,
FileMode.Append,
FileSystemRights.AppendData,
FileShare.ReadWrite,
FileShare.ReadWrite | FileShare.Delete,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

File deletion was enabled by adding this, but I don't know if it's the best way to do this.
Should we make another config entry to only enable deletion when required?

Comment on lines +33 to +35
TextWriter _output;
FileStream _underlyingStream;
readonly string _path;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed to make _output and _underlyingStream non-readonly, so I could dispose them and recreate the file after it was deleted.
Also added _path variable to be re-used when recreating the file.

Comment on lines +85 to +90
if (!System.IO.File.Exists(_path))
{
_underlyingStream.Dispose();
_underlyingStream = System.IO.File.Open(_path, FileMode.Append, FileAccess.Write, FileShare.ReadWrite | FileShare.Delete);
_output = new StreamWriter(_underlyingStream, new UTF8Encoding(encoderShouldEmitUTF8Identifier: false));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I'm worried about the performance of this File.Exists(), this was how I managed to make the file be re-created. If you guys have another idea of how to make this verification, please let me know.
I also noticed that this chunk must be before _underlyingStream.Seek(0, SeekOrigin.End); in order to create the file correctly after is deleted.

@thiagosgarcia thiagosgarcia marked this pull request as ready for review April 28, 2020 21:25
@thiagosgarcia
Copy link
Contributor Author

I noticed that the tests are not passing with VS2019 image. I'm going to check this as soon as I can.

@amoerie
Copy link

amoerie commented May 20, 2021

@thiagosgarcia @nblumhardt Any update on this issue? Do you need help?

@thiagosgarcia
Copy link
Contributor Author

Hey! I'm sorry, I've been busy these last months and completely forgot about this.
I recall that I did have some design concerns (written in description), but I'm not quite sure if it still stands. I'm gonna update the code, re run everything and address any question I have in the next few days :)

@amoerie
Copy link

amoerie commented May 20, 2021

That's great to hear. I'm willing to help out where I can, but I'm new to the Serilog codebase so I can't promise anything but time and goodwill. 😉

@nblumhardt
Copy link
Member

Hey all! Looks like this PR has stalled; #186 might offer a partial solution as long as you're not using the shared version of the sink?

Doing some housekeeping and clearing out the list of open PRs to keep things manageable; if you do decide to pick it back up, @thiagosgarcia, please just let me know and we can reopen 👍

@nblumhardt nblumhardt closed this Jun 22, 2021
@ShitalPawarCC
Copy link

This is a much needed feature. Can someone fix this issue and merge the PR after testing?

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.

4 participants