Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 13 commits into
base: dev
Choose a base branch
from
Open

Feature/support atomic append #346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
bb22663
updated to use FileSystemAclExtensions.Create() to append to file
cortex-bt Mar 20, 2025
6cf1330
added net7 as well
cortex-bt Mar 20, 2025
ccd907b
added to list
cortex-bt Mar 20, 2025
af13b14
removed net7 for now
cortex-bt Mar 20, 2025
3245d2f
no need for checks
cortex-bt Mar 20, 2025
93f873f
ensuring net6 uses os_mutex
cortex-bt Mar 20, 2025
4627ad3
checking for platform required for .net8 to work
cortex-bt Mar 20, 2025
7691dcb
creating new branch as we are only interested on .net8
cortex-bt Mar 24, 2025
89bc34f
removed whitespace
cortex-bt Mar 24, 2025
6822f79
added tests for atomic operations
cortex-bt Mar 24, 2025
7141e6d
added tests for multiple users accessing file sink writes to
cortex-bt Mar 24, 2025
c13bb1a
simplified statements
cortex-bt Mar 26, 2025
8e7ba53
shoulde be a == instead of !=
cortex-bt Mar 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/Serilog.Sinks.File/Serilog.Sinks.File.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,25 +25,25 @@
<DefineConstants>$(DefineConstants);ATOMIC_APPEND;HRESULTS</DefineConstants>
</PropertyGroup>

<PropertyGroup Condition=" '$(TargetFrameworkIdentifier)' != '.NETFramework' ">
<PropertyGroup Condition=" '$(TargetFrameworkIdentifier)' == '.netstandard2.0'">
<DefineConstants>$(DefineConstants);OS_MUTEX</DefineConstants>
</PropertyGroup>

<PropertyGroup Condition=" '$(TargetFramework)' == 'net6.0' ">
<DefineConstants>$(DefineConstants);ENUMERABLE_MAXBY</DefineConstants>
<DefineConstants>$(DefineConstants);ENUMERABLE_MAXBY;OS_MUTEX</DefineConstants>
</PropertyGroup>

<PropertyGroup Condition=" '$(TargetFramework)' == 'net8.0' ">
<DefineConstants>$(DefineConstants);ENUMERABLE_MAXBY</DefineConstants>
<DefineConstants>$(DefineConstants);ENUMERABLE_MAXBY;ATOMIC_APPEND</DefineConstants>
</PropertyGroup>

<PropertyGroup Condition=" '$(TargetFramework)' == 'net9.0' ">
<DefineConstants>$(DefineConstants);ENUMERABLE_MAXBY</DefineConstants>
<DefineConstants>$(DefineConstants);ENUMERABLE_MAXBY;ATOMIC_APPEND</DefineConstants>
</PropertyGroup>

<ItemGroup>
<None Include="..\..\assets\serilog-sink-nuget.png" Pack="true" Visible="false" PackagePath="/" />
<None Include="..\..\README.md" Pack="true" Visible="false" PackagePath="/" />
</ItemGroup>

</Project>
</Project>
49 changes: 45 additions & 4 deletions src/Serilog.Sinks.File/Sinks/File/SharedFileSink.AtomicAppend.cs
View file
Open in desktop
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.
Expand All @@ -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;
Expand Down Expand Up @@ -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();
}
Copy link
Member

Choose a reason for hiding this comment

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

This will break shared flag support on non-Windows platforms; we'd need to fall back to the OS mutex variant in the non-Windows cases.

Copy link
Author

@cortex-bt cortex-bt Mar 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The 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))
{
_fileOutput = CreateFile(
_path,
FileMode.Append,
FileSystemRights.AppendData,
FileShare.ReadWrite,
length,
FileOptions.None);
_fileStreamBufferLength = length;
}
else
{
_fileOutput = new FileStream(
_path,
FileMode.Append,
FileSystemRights.AppendData,
FileShare.ReadWrite,
length,
FileOptions.None);
}

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Author

@cortex-bt cortex-bt Mar 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

so then something like this in the csproj would be more appropriate

image

So for windows platforms it enables ATOMIC_APPEND and for non-Windows OS_MUTEX

Copy link
Member

Choose a reason for hiding this comment

The 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 net8.0 and net9.0 binaries need to run on both platforms. The example above would just make the package work only for the same OS as the build server.

Copy link
Author

@cortex-bt cortex-bt May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The 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,
Expand All @@ -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();
}
Expand Down Expand Up @@ -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
98 changes: 97 additions & 1 deletion test/Serilog.Sinks.File.Tests/SharedFileSinkTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using Serilog.Core;
using Serilog.Core;
using Xunit;
using Serilog.Formatting.Json;
using Serilog.Sinks.File.Tests.Support;
Expand Down Expand Up @@ -94,4 +94,100 @@ public void WhenLimitIsNotSpecifiedFileSizeIsNotRestricted()
var size = new FileInfo(path).Length;
Assert.True(size > maxBytes * 2);
}

[Fact]
public void FileIsNotLockedAfterDisposal()
{
using var tmp = TempFolder.ForCaller();
var path = tmp.AllocateFilename("txt");
var evt = Some.LogEvent("Hello, world!");

using (var sink = new SharedFileSink(path, new JsonFormatter(), null))
{
sink.Emit(evt);
}

// Ensure the file is not locked after the sink is disposed
var exceptionThrown = false;
try
{
using (var stream = new FileStream(path, FileMode.Open, FileAccess.ReadWrite))
{
}
}
catch (IOException)
{
exceptionThrown = true;
}

Assert.False(exceptionThrown, "File should not be locked after sink disposal.");
}

[Fact]
public void FileIsLockedByOneUserAndAnotherUserTriesToWrite()
{
using var tmp = TempFolder.ForCaller();
var path = tmp.AllocateFilename("txt");
var evt = Some.LogEvent("Hello, world!");

// Lock the file by one user
using (var stream = new FileStream(path, FileMode.Create, FileAccess.ReadWrite, FileShare.None))
{
using (var writer = new StreamWriter(stream))
{
writer.WriteLine("Initial content");
writer.Flush();

// Try to write to the locked file by another user
var exceptionThrown = false;
try
{
using (var sink = new SharedFileSink(path, new JsonFormatter(), null))
{
sink.Emit(evt);
}
}
catch (IOException)
{
exceptionThrown = true;
}

Assert.True(exceptionThrown, "IOException should be thrown when trying to write to a locked file.");
}
}

// Verify the file content
var lines = System.IO.File.ReadAllLines(path);
Assert.Contains("Initial content", lines);
Assert.DoesNotContain("Hello, world!", lines);
}

[Fact]
public async Task FileIsNotLockedDuringAsyncOperations()
{
using var tmp = TempFolder.ForCaller();
var path = tmp.AllocateFilename("txt");
var evt = Some.LogEvent("Hello, world!");

using (var sink = new SharedFileSink(path, new JsonFormatter(), null))
{
await Task.Run(() => sink.Emit(evt));
}

// Ensure the file is not locked after async operations
var exceptionThrown = false;
try
{
using (var stream = new FileStream(path, FileMode.Open, FileAccess.ReadWrite))
{
stream.ReadAllLines();
}
}
catch (IOException)
{
exceptionThrown = true;
}

Assert.False(exceptionThrown, "File should not be locked after async operations.");
}
}