-
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
[FileStream] Flush(flushToDisk: true) should always flush to disk #77384
Conversation
Tagging subscribers to this area: @dotnet/area-system-io Issue Detailsfixes #77373 6.0 implementation for reference: Lines 268 to 276 in fe21795
I've tried really hard to write a reliable test for this bug, but I've failed to do that in reasonable period of time. Based on the docs:
I've tried to find some way of writing and reading the metadata and using
|
// If the underlying strategy is not seekable AND we have something in the read buffer, then FlushRead would throw. | ||
// We can either throw away the buffer resulting in data loss (!) or ignore the Flush. | ||
// We cannot throw because it would be a breaking change. We opt into ignoring the Flush in that situation. | ||
return; |
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.
Why shouldn't / can't we still FSync / FlushFileBuffers in this case?
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.
Why shouldn't / can't we still FSync / FlushFileBuffers in this case?
I would expect it to throw for pipes. I am going to check it and get back to you.
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.
@stephentoub it turned out to be possible, moreover we even already have tests for that:
runtime/src/libraries/System.IO.FileSystem/tests/FileStream/Flush.cs
Lines 146 to 162 in 57bfe47
[Theory] | |
[InlineData(null)] | |
[InlineData(false)] | |
[InlineData(true)] | |
[SkipOnPlatform(TestPlatforms.Browser, "IO.Pipes not supported")] | |
public void FlushCanBeUsedOnPipes_Success(bool? flushToDisk) | |
{ | |
using (var pipeStream = new AnonymousPipeServerStream(PipeDirection.In)) | |
using (var clientHandle = pipeStream.ClientSafePipeHandle) | |
{ | |
SafeFileHandle handle = new SafeFileHandle((IntPtr)int.Parse(pipeStream.GetClientHandleAsString()), false); | |
using (FileStream fs = new FileStream(handle, FileAccess.Write, 1, false)) | |
{ | |
Flush(fs, flushToDisk); | |
} | |
} | |
} |
I've updated the code.
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.
Thanks for checking.
…n-seekable files (it does not throw)
fixes #77373
5.0 implementation for reference:
runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.cs
Lines 268 to 276 in fe21795
I've tried really hard to write a reliable test for this bug, but I've failed to do that in reasonable period of time.
Based on the docs:
I've tried to find some way of writing and reading the metadata and using
Flush(true)
to flush the metadata, but I was unable to find a simple and reliable solution for doing that (the metadata that we expose is refreshed without the need to flush to disk, other metdata is hard to get and either requires some dirty COM code or 3rd party or .NET Framework-only dependency to do it).