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

Feature/support atomic append #346

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

Conversation

cortex-bt
Copy link

@cortex-bt cortex-bt commented Mar 24, 2025

This pr is to fix the issue/solution raised on this thread #221

@cortex-bt cortex-bt marked this pull request as ready for review March 24, 2025 16:08
Copy link
Member

@nblumhardt nblumhardt left a comment

Choose a reason for hiding this comment

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

Thanks for sending this! I might be misunderstanding how the patch works, but looks like this will break the .NET 8/9 builds on other platforms by enabling atomic append, but throwing in the platform check?

Separate note, we'll also need to think about what this might mean for partially-updated applications: writers using atomic append, and writers using the OS mutex, will not coordinate. This might need some API design consideration, e.g. by keeping platform defaults in the shared: true overload of WriteTo.File(), but perhaps adding an overload that accepts a sharing model? Needs some thought. Does seem to align with having both variants of the shared file sink in the same binary, if we do that to handle the non-Windows case mentioned above.

@@ -25,7 +25,7 @@
<DefineConstants>$(DefineConstants);ATOMIC_APPEND;HRESULTS</DefineConstants>
</PropertyGroup>

<PropertyGroup Condition=" '$(TargetFrameworkIdentifier)' != '.NETFramework' ">
<PropertyGroup Condition=" '$(TargetFrameworkIdentifier)' != '.NETFramework' AND '$(TargetFramework)' == 'net6.0'">
Copy link
Member

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 than AND. To keep things simple, perhaps we just put OS_MUTEX in the net6.0 property group below, add a block for netstandard2.0, and remove this block?

Copy link
Author

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

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

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

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.

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.

2 participants