-
Notifications
You must be signed in to change notification settings - Fork 125
Feature/support atomic append #346
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
base: dev
Are you sure you want to change the base?
Changes from 11 commits
bb22663
6cf1330
ccd907b
af13b14
3245d2f
93f873f
4627ad3
7691dcb
89bc34f
6822f79
7141e6d
c13bb1a
8e7ba53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
// Copyright 2013-2019 Serilog Contributors | ||
// Copyright 2013-2019 Serilog Contributors | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
|
@@ -14,6 +14,9 @@ | |
|
||
#if ATOMIC_APPEND | ||
|
||
using System.IO; | ||
using System.Runtime.InteropServices; | ||
using System.Runtime.Versioning; | ||
using System.Security.AccessControl; | ||
using System.Text; | ||
using Serilog.Core; | ||
|
@@ -77,13 +80,20 @@ public SharedFileSink(string path, ITextFormatter textFormatter, long? fileSizeL | |
|
||
// FileSystemRights.AppendData sets the Win32 FILE_APPEND_DATA flag. On Linux this is O_APPEND, but that API is not yet | ||
// exposed by .NET Core. | ||
_fileOutput = new FileStream( | ||
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
{ | ||
_fileOutput = CreateFile( | ||
path, | ||
FileMode.Append, | ||
FileSystemRights.AppendData, | ||
FileShare.ReadWrite, | ||
_fileStreamBufferLength, | ||
FileOptions.None); | ||
} | ||
else | ||
{ | ||
throw new NotSupportedException(); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will break There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about reverting back to using new FileStream in the if statement for no windows platforms. So net8/9 would work with the CreateFile method while the rest still work as before using the FileStream ? e.g if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The OS mutex would still need to be used; the machinery around atomic append and the mutex ensure writes are not overlapping, which otherwise creates garbled logs when multiple processes share the same log file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CSPROJ settings like that are applied at build time, but the same packaged There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have any recommendations on how we could fall back to the OS mutex variant in the non-Windows cases? Or is this something that needs further planning? |
||
|
||
_writeBuffer = new MemoryStream(); | ||
_output = new StreamWriter(_writeBuffer, | ||
|
@@ -105,15 +115,21 @@ bool IFileSink.EmitOrOverflow(LogEvent logEvent) | |
if (length > _fileStreamBufferLength) | ||
{ | ||
var oldOutput = _fileOutput; | ||
|
||
_fileOutput = new FileStream( | ||
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
{ | ||
_fileOutput = CreateFile( | ||
_path, | ||
FileMode.Append, | ||
FileSystemRights.AppendData, | ||
FileShare.ReadWrite, | ||
length, | ||
FileOptions.None); | ||
_fileStreamBufferLength = length; | ||
} | ||
else | ||
{ | ||
throw new NotSupportedException(); | ||
} | ||
|
||
oldOutput.Dispose(); | ||
} | ||
|
@@ -188,6 +204,31 @@ void ISetLoggingFailureListener.SetFailureListener(ILoggingFailureListener failu | |
{ | ||
_failureListener = failureListener ?? throw new ArgumentNullException(nameof(failureListener)); | ||
} | ||
|
||
private static FileStream CreateFile(string path, FileMode mode, FileSystemRights rights, FileShare share, int bufferSize, FileOptions options) | ||
{ | ||
// FileSystemRights.AppendData sets the Win32 FILE_APPEND_DATA flag. On Linux this is O_APPEND | ||
#if NET48 | ||
_fileOutput = new FileStream(path, mode, rights, share, bufferSize, options); | ||
#else | ||
// In .NET 7 for Windows it's exposed with FileSystemAclExtensions.Create API | ||
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
{ | ||
var _fileOutput = FileSystemAclExtensions.Create(new FileInfo(path), mode, rights, share, bufferSize, options, new FileSecurity()); | ||
|
||
// Inherit ACL from container | ||
var security = new FileSecurity(); | ||
security.SetAccessRuleProtection(false, false); | ||
FileSystemAclExtensions.SetAccessControl(new FileInfo(path), security); | ||
|
||
return _fileOutput; | ||
} | ||
else | ||
{ | ||
throw new NotSupportedException(); | ||
} | ||
#endif | ||
} | ||
} | ||
|
||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be
OR
rather thanAND
. To keep things simple, perhaps we just putOS_MUTEX
in thenet6.0
property group below, add a block fornetstandard2.0
, and remove this block?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed. Have made those changes now