Use O_APPEND and FILE_APPEND_DATA to allow for atomic file appends

[adamsitnik]

So far, our FileMode.Append implementation was always seeking to the end of file, saving it as the current position and later using this position for writes. It was assuming that nobody else can be writing to the same file (which does not need to always be the true).

Moreover, none of the FileStream ctors that accept handle accepts a FileMode. This information was missing, and FileStream created from handle opened with O_APPEND or FILE_APPEND_DATA could not recognize that it's supposed to append to the end of file.

By using O_APPEND and FILE_APPEND_DATA we can force the OS to perform atomic writes to the end of file for us.

This can be combined with disabled buffering (bufferSize == 0) and allow for atomic appends to file, which could be very useful for some loggers like serilog/serilog-sinks-file#221

fixes #53432

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

---

So far, our FileMode.Append implementation was always seeking to the end of file, saving it as the current position and later using this position for writes. It was assuming that nobody else can be writing to the same file (which does not need to always be the truth).

Moreover, none of the FileStream ctors that accept handle accepts a FileMode. This information was missing, and FileStream created from handle opened with O_APPEND or FILE_APPEND_DATA could not recognize that it's supposed to append to the end of file.

By using O_APPEND and FILE_APPEND_DATA we can force the OS to perform atomic writes to the end of file for us.

This can be combined with disabled buffering (bufferSize == 0) and allow for atomic appends to file, which could be very useful for some loggers like serilog/serilog-sinks-file#221

fixes #53432

Author:	adamsitnik
Assignees:	-
Labels:	area-System.IO
Milestone:	-

[stephentoub]

Doesn't this add yet another syscall in some cases (in particular for a new SafeFileHandle(fd, ...)-constructed handle? Is that really worth it?

[stephentoub]

Does RandomAccess.WriteAtOffsetAsync guarantee to write out everything it was given, or might it write less? If the latter, I'm realizing now that both this and the UnixFileStreamStrategy are potentially broken, in that their WriteAsync methods could then end up doing only a partial write even though the stream contract guarantees everything will be written.

[stephentoub]

I don't understand this logic. On both .NET Framework 4.8 and .NET 5, this:

using System;
using System.IO;

class Program
{
    static void Main()
    {
        const string Path = @"tmp.txt";
        File.Delete(Path);

        using (var fs = new FileStream(Path, FileMode.Append))
        {
            fs.Write(new byte[] { 1, 2, 3, 4, 5 }, 0, 5);
            fs.Position = 1;
            Console.WriteLine(fs.Position);
            Console.WriteLine(fs.Length);
        }
    }
}

prints:

1
5

With the logic you have here, won't this print:

5
5

?

[stephentoub]

I also don't understand this logic. This program:

using System;
using System.IO;
using System.Text;

class Program
{
    static void Main()
    {
        const string Path = @"tmp.txt";
        File.Delete(Path);

        using (var fs = new FileStream(Path, FileMode.Append))
        {
            fs.Write(Encoding.ASCII.GetBytes("hello"));
            fs.Position = 1;
            fs.Write(Encoding.ASCII.GetBytes("abc"));
        }

        Console.WriteLine(File.ReadAllText(Path));
    }
}

today prints:

habco

highlighting that a) you are allowed to seek backwards (just not before the position when the file was opened) and b) it's not losing all data after the seek point.

[stephentoub]

Similarly, this seems wrong. Today this:

using System;
using System.IO;
using System.Text;

class Program
{
    static void Main()
    {
        const string Path = @"tmp.txt";
        File.Delete(Path);

        using (var fs = new FileStream(Path, FileMode.Append))
        {
            fs.Write(Encoding.ASCII.GetBytes("hello"));
            fs.SetLength(1);
        }

        Console.WriteLine(File.ReadAllText(Path));
    }
}

prints

h

but it seems like this code will end up throwing.

[adamsitnik]

I am going to close the PR right now and re-open it when I am back from vacations.

[adamsitnik]

The official doc for FileMode.Append says:

Trying to seek to a position before the end of the file throws an IOException exception

So setting Postion to anything that is less that Length should throw. I believe that existing implementation was incomplete.

The position was captured when file was being opened:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs

Lines 48 to 55 in b75e55b

	private void Init(FileMode mode, string originalPath, FileOptions options)
	{
	Debug.Assert(!_useAsyncIO || _fileHandle.ThreadPoolBinding != null);
	// For Append mode...
	if (mode == FileMode.Append)
	{
	_appendStart = SeekCore(_fileHandle, 0, SeekOrigin.End);

We were trying to prevent from truncating the file:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs

Lines 253 to 254 in b75e55b

	if (_appendStart != -1 && value < _appendStart)
	throw new IOException(SR.IO_SetLengthAppendTruncate);

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs

Lines 415 to 421 in b75e55b

	// Prevent users from overwriting data in a file that was opened in
	// append mode.
	if (_appendStart != -1 && pos < _appendStart)
	{
	SeekCore(_fileHandle, oldPos, SeekOrigin.Begin);
	throw new IOException(SR.IO_SeekAppendOverwrite);
	}

The problem is that we were never updating the _appendStart value which should be happening after each write.

The way I understand FileMode.Append: always append to the end of file, don't allow for truncation or for overwriting the data.

@JeremyKuhne @stephentoub is my understanding correct?

[stephentoub]

I don't agree with the interpretation. I read the docs as being about seeking to earlier than the position when the file was opened, which is exactly what the implementation supports. Changing it is a breaking change, not only in behavior but also in required permissions.

[adamsitnik]

also in required permissions.

Could you please explain what do you mean by that?

[snakefoot]

@adamsitnik I can understand the wish to make the FileStream-contract predictable, when applying FILE_APPEND_DATA / O_APPEND where Operating System always appends to end (and ignore any file-seeks or change of file-position).

When the user has opened a file with FileMode.Append then I think the user will accept that even if the doing custom file-seeks, then all writes will be appended to the end (independent of current file-position).

But it will be confusing for the user if not possible to make random reads. For me FileMode.Append means always write to the end of the file, but still allow random reads.

• When making a file-write then FileStream-Position should automatically move to the end when FileMode.Append (Perform the write and then update FileStream-Position)
  • For performance reasons then one could make internal filestream-position nullable, and file-write resets the filestream-position to null. When checking FileStream-Position and it is null, then it will assign it to FileStream-Length.
• When checking current FileStream-Position then it will remain unchanged (even if other processes might have appended to the file).
• When making a file-read then it will always use the current FileStream-Position

[JeremyKuhne]

I think there is some unfortunate concept overlap between specific rights and "position at the end of the file". It looks as if we were trying to emulate FILE_APPEND_DATA's behavior over GENERIC_WRITE, and not quite getting it right.

I wouldn't change the existing behavior. My opinion is clarify the docs and add a new option if we think differing functionality is important.

[stephentoub]

Could you please explain what do you mean by that?

I mean FILE_APPEND_DATA is a specific access right:
https://docs.microsoft.com/en-us/windows/win32/fileio/file-access-rights-constants
representing "the right to append data to the file". I don't know what the ramifications in this regard would be of suddently changing FileMode.Append to specify that for a file object that might not allow it.

[snakefoot]

In NetFramework then FILE_APPEND_DATA is applied when using the FileStream-constructor with FileSystemRights.AppendData

[adamsitnik]

I don't know what the ramifications in this regard would be of suddently changing FileMode.Append to specify that for a file object that might not allow it.

We are currently using GENERIC_WRITE:

runtime/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs

Lines 64 to 66 in 1be8fd8

	int fAccess =
	((access & FileAccess.Read) == FileAccess.Read ? Interop.Kernel32.GenericOperations.GENERIC_READ : 0) |
	((access & FileAccess.Write) == FileAccess.Write ? Interop.Kernel32.GenericOperations.GENERIC_WRITE : 0);

runtime/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GenericOperations.cs

Line 11 in 1be8fd8

internal const int GENERIC_WRITE = 0x40000000;

which already contains FILE_APPEND_DATA amongst many other things:

runtime/src/libraries/Common/src/Interop/Windows/NtDll/Interop.NtCreateFile.cs

Lines 594 to 600 in 57bfe47

	/// <summary>
	/// Maps internally to <see cref="FILE_APPEND_DATA"/> | <see cref="FILE_WRITE_ATTRIBUTES"/> | <see cref="FILE_WRITE_DATA"/>
	/// | <see cref="FILE_WRITE_EA"/> | <see cref="STANDARD_RIGHTS_READ"/> | <see cref="SYNCHRONIZE"/>.
	/// (For directories, <see cref="FILE_ADD_SUBDIRECTORY"/> | <see cref="FILE_WRITE_ATTRIBUTES"/> | <see cref="FILE_ADD_FILE"/> AddFile
	/// | <see cref="FILE_WRITE_EA"/> | <see cref="STANDARD_RIGHTS_READ"/> | <see cref="SYNCHRONIZE"/>.)
	/// </summary>
	FILE_GENERIC_WRITE = 0x40000000, // GENERIC WRITE

I am not sure if contains is the right word, but it's more or less:

FILE_GENERIC_WRITE  =  FILE_APPEND_DATA | FILE_WRITE_ATTRIBUTES | FILE_WRITE_DATA | FILE_WRITE_EA | STANDARD_RIGHTS_READ and so on...

So permissions would not be a problem

[adamsitnik]

I wouldn't change the existing behavior. My opinion is clarify the docs and add a new option if we think differing functionality is important.

You are right. Let me prepare a proposal. I am going to convert this PR into a draft and add NO MERGE label until then.

[adamsitnik]

Done: #53432 (comment)

[ghost]

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.