-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Use O_APPEND and FILE_APPEND_DATA to allow for atomic file appends #55465
Conversation
# Conflicts: # src/libraries/System.IO.FileSystem/tests/Net5CompatTests/System.IO.FileSystem.Net5Compat.Tests.csproj
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsSo far, our Moreover, none of the By using This can be combined with disabled buffering ( fixes #53432
|
src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs
Outdated
Show resolved
Hide resolved
|
||
// POSIX requires that pwrite should respect provided offset even for handles opened with O_APPEND. | ||
// But Linux and BSD don't do that, moreover their behaviour is different. So we always use write for O_APPEND. | ||
int result = handle.CanSeek && !handle.IsAppend ? |
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.
Doesn't this add yet another syscall in some cases (in particular for a new SafeFileHandle(fd, ...)
-constructed handle? Is that really worth it?
WriteAsyncCore(source, cancellationToken).AsValueTask(); | ||
#pragma warning restore CA2012 | ||
|
||
private ValueTask<int> WriteAsyncCore(ReadOnlyMemory<byte> source, CancellationToken cancellationToken) |
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.
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.
|
||
public override long Position | ||
{ | ||
get => Length; |
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.
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
?
// Prevent users from overwriting data in a file that was opened in append mode. | ||
if (newLength < length) | ||
{ | ||
throw new IOException(SR.IO_SeekAppendOverwrite); | ||
} | ||
|
||
FileStreamHelpers.SetFileLength(_fileHandle, newLength); |
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.
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.
if (value < Length) | ||
{ | ||
throw new IOException(SR.IO_SetLengthAppendTruncate); | ||
} |
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.
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.
I am going to close the PR right now and re-open it when I am back from vacations. |
# Conflicts: # src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs # src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs # src/libraries/System.Private.CoreLib/src/System/IO/Strategies/OSFileStreamStrategy.cs
…Exception exception" - the test should verify the intended behavior, not buggy implementation
…andles/SafeFileHandle.Unix.cs Co-authored-by: Stephen Toub <[email protected]>
The official doc for
So setting The position was captured when file was being opened: Lines 48 to 55 in b75e55b
We were trying to prevent from truncating the file: Lines 253 to 254 in b75e55b
Lines 415 to 421 in b75e55b
The problem is that we were never updating the The way I understand @JeremyKuhne @stephentoub is my understanding correct? |
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. |
Could you please explain what do you mean by that? |
@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 But it will be confusing for the user if not possible to make random reads. For me
|
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 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. |
I mean FILE_APPEND_DATA is a specific access right: |
In NetFramework then FILE_APPEND_DATA is applied when using the FileStream-constructor with FileSystemRights.AppendData |
We are currently using Lines 64 to 66 in 1be8fd8
runtime/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GenericOperations.cs Line 11 in 1be8fd8
which already contains runtime/src/libraries/Common/src/Interop/Windows/NtDll/Interop.NtCreateFile.cs Lines 594 to 600 in 57bfe47
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 |
You are right. Let me prepare a proposal. I am going to convert this PR into a draft and add |
Done: #53432 (comment) |
Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it. |
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 aFileMode
. This information was missing, andFileStream
created from handle opened withO_APPEND
orFILE_APPEND_DATA
could not recognize that it's supposed to append to the end of file.By using
O_APPEND
andFILE_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#221fixes #53432