Implement RandomAccess.FlushToDisk() #89100

ericmutta · 2023-07-18T16:17:38Z

dotnet-issue-labeler · 2023-07-18T16:17:42Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2023-07-18T16:17:58Z

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

Issue Details

Fix #86836

Author:	ericmutta
Assignees:	-
Labels:	`area-System.IO`, `new-api-needs-documentation`
Milestone:	-

danmoseley · 2023-07-18T16:27:10Z

src/libraries/System.Runtime/ref/System.Runtime.cs

@@ -10023,6 +10023,7 @@ public static partial class RandomAccess
        public static void Write(Microsoft.Win32.SafeHandles.SafeFileHandle handle, System.ReadOnlySpan<byte> buffer, long fileOffset) { }
        public static System.Threading.Tasks.ValueTask WriteAsync(Microsoft.Win32.SafeHandles.SafeFileHandle handle, System.Collections.Generic.IReadOnlyList<System.ReadOnlyMemory<byte>> buffers, long fileOffset, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
        public static System.Threading.Tasks.ValueTask WriteAsync(Microsoft.Win32.SafeHandles.SafeFileHandle handle, System.ReadOnlyMemory<byte> buffer, long fileOffset, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
+        public static void FlushToDisk(Microsoft.Win32.SafeHandles.SafeFileHandle handle) { throw null; }


nit - please put in alpha order. we have tools that regenerate this and it will otherwise cause a diff

OK...before I do, I notice that SetLength appears before the various Read and ReadAsync methods, when it should appear after them in order to preserve alpha order. Should I make that change in this PR too or is it best to use a separate issue?

If you run the tool it will take care of it

https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/updating-ref-source.md

src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.cs

This "platform-depedent" should read "platform-dependent". Co-authored-by: Dan Moseley <danmose@microsoft.com>

Declarations of class members in System.Runtime.cs should be in alphabetic order (see dotnet#89100 (comment)). Simplify comments on the call to "FileStreamHelpers.FlushToDisk()" (see dotnet#89100 (comment)).

adamsitnik

@ericmutta great job, thank you for your contribution! PTAL at my comments

src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.cs

src/libraries/System.IO.FileSystem/tests/RandomAccess/FlushToDisk.Unix.cs

src/libraries/System.IO.FileSystem/tests/RandomAccess/FlushToDisk.cs

src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs

ericmutta · 2023-07-19T17:23:30Z

Many thanks @adamsitnik for the detailed and encouraging feedback! I will do the necessary over the next 48 hours and push more commits 👍

danmoseley · 2023-07-19T18:10:37Z

src/libraries/System.IO.FileSystem/tests/RandomAccess/FlushToDisk.cs

+                RandomAccess.Write(handle, randomBytes, fileOffset: 0);
+
+                // Get the file time BEFORE flushing to disk.
+                DateTime fileTimeBeforeFlush = File.GetLastWriteTimeUtc(testFilePath);


consider setting a last write time here that's a little older. otherwise you'll fail when the file system timestamp granularity isn't high enough. at an extreme, this test will fail on a FAT32 partition.

also, this test will fail if a daylight savings change or leap second happens during the test. obviously, that's unlikely, but several of our tests fail every time there's such an event as there's zillions of tests and they run all the time. to fix that, you can set a date that's a year ago, for example.

Interesting points @danmoseley! In this test we are not explicitly setting the last write time so I can't control it in any way. When the file is flushed, it is the operating system that updates the last write time and the test is just checking that the time after flushing is greater than the time before flushing (or that they are at least equal, as explained in the comment near the Assert at the end).

The most important comment in the test says:

// Flush the file to disk. As a bare minimum test, this should work without throwing an // exception on all supported platforms.

...i.e. we are mostly interested in the fact that flushing works without throwing an exception on all platforms. So I can remove all the date-related asserts to avoid the issues you raised. Please let me know if I should do this 👍

danmoseley · 2023-07-19T18:13:26Z

src/libraries/System.IO.FileSystem/tests/RandomAccess/FlushToDisk.cs

+                // to. This "implicit flush" would also update the file time and if it happens BEFORE we set the
+                // fileTimeBeforeFlush variable above, then both fileTimeBeforeFlush and fileTimeAfterFlush will
+                // end up with the same value, which is why we use the ">=" operator below instead of using ">".
+                Assert.True(fileTimeAfterFlush >= fileTimeBeforeFlush);


another reason to set your own older timestamp before the flush is you eliminate this race. and then you can use Assert.NotEqual here instead. which also has the advantage that if it fails, it will be clear why, whereas when you use Assert.True it just says false expected true.

Please see this response, I believe the decision we make there will address this too 👍

src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs

This includes comments that: 1. Can become outdated if methods are renamed (see dotnet#89100 (comment)). 2. Are not absolutely critical (see dotnet#89100 (comment)). I also removed the mention of using FileOptions.WriteThrough as an alternative to RandomAccess.FlushToDisk(). This is an advanced scenario that can actually degrade performance if done incorrectly (i.e. if the user doesn't manage their own buffers such that all writes keep hitting the disk) so it's probably not wise to mention it unless we go into more detail than is ideal for inclusion in the Remarks section of an XML doc comment. To remove the lengthy comment in the ValidateInput() function I had to rename the optional parameter from "flushingToDisk" to the more generic and self-describing "allowUnseekableHandles" (see dotnet#89100 (comment)).

src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.cs

The differences between Unix and Windows when it comes to flushing unseekable handles is worth noting explicitly and the comment that did this was previously deleted but has now been restored with some minor edits to fit its new context (see dotnet@a7d6289#r1270505888).

See the following discussions for the rationale: - dotnet#89100 (comment) - dotnet#89100 (comment) NOTE: the [MemberData] used by the theories returns one element in order to reduce the number of tests that need to run per build. Since the element is random, different builds will likely pick a different value allowing more variations to be tested over many build cycles.

ericmutta · 2023-07-21T17:40:14Z

Thanks @danmoseley and @adamsitnik for all the feedback on this! I have addressed most of the issues and will be back to resolve the rest over the next few days (contributing to .NET is dangerously addictive, I wish I could do it full time...but until then I have to restrain myself and attend to the day job before I get fired 😄 🏃 )

src/libraries/System.IO.FileSystem/tests/RandomAccess/FlushToDisk.cs

adamsitnik

It looks very good, we are almost there 👍

Again thank you for your contribution @ericmutta !

src/libraries/System.IO.FileSystem/tests/RandomAccess/FlushToDisk.cs

src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs

See dotnet#89100 (comment)

See the following: - dotnet#89100 (comment) - dotnet#89100 (comment)

adamsitnik

@ericmutta apologies for the delay

src/libraries/System.IO.FileSystem/tests/RandomAccess/FlushToDisk.Unix.cs

There is a difference in behaviour between FlushFileBuffers() on Windows and fsync() on Unix when it comes to read-only files. On Windows it fails with ERROR_ACCESS_DENIED while on Unix it just works. We made a decision to harmonize the platforms by making Windows behave like Unix (i.e. we catch and ignore the error if a user attempts to flush a file opened for reading, so it is effectively a no-op). See dotnet#89100 (comment)

adamsitnik

LGTM, thank you for your contribution @ericmutta !

adamsitnik · 2023-08-07T11:53:35Z

@jeffhandley I request for the permission to merge. This PR adds FlushToDisk method to RandomAccess class. It was already present for years on FileStream, it reuses an existing helper. So the risk for breaking anything else is very small.

jeffhandley

I approve of merging this in for .NET 8 RC1.

It adds a targeted new API that was approved a couple months ago, reusing existing code to implement the logic, and there's sufficient testing added.

adamsitnik · 2023-08-07T18:12:02Z

The failure is unrelated (#87879), merging!

ericmutta · 2023-08-07T19:32:07Z

The week is officially off to a great start :-)

My thanks to @danmoseley for encouraging me to take a stab at this; to the API Review Board for approving the name; to @adamsitnik for the many time-saving tips, pointers and code snippets; to both @danmoseley and @adamsitnik for helpful feedback during PR review; and to @jeffhandley for the final review required to merge. You guys are awesome and this has been a fantastic experience! 🚀

danmoseley · 2023-08-07T22:13:26Z

Thanks @ericmutta ! We hope you will become a regular contributor here.

ericmutta · 2023-08-08T11:51:37Z

Thanks @danmoseley!

I hope to be back after we ship .NET 8 later this year, to contribute to .NET 9.

As part of the database-like project that led me to request RandomAccess.FlushToDisk(), I have had to build some things from scratch that I wish were already in the .NET libraries. Currently, the SemaphoreSlim class is the only synchronization primitive in the System.Threading namespace that has some async support. If there's any interest from the .NET team, I would like to take a stab at adding more async support to other primitives like ReaderWriterLockSlim during the .NET 9 time frame next year 👍

adamsitnik · 2023-08-08T11:56:08Z

Currently, the SemaphoreSlim class is the only synchronization primitive in the System.Threading namespace that has some async support. If there's any interest from the .NET team, I would like to take a stab at adding more async support to other primitives like ReaderWriterLockSlim during the .NET 9 time frame next year

cc @stephentoub

danmoseley · 2023-08-10T22:51:22Z

If there's any interest from the .NET team, I would like to take a stab at adding more async support to other primitives like ReaderWriterLockSlim during the .NET 9 time frame next year 👍

the best way to see whether there's interest is to start an API proposal. Be sure to search existing issues though, stuff like this has likely been proposed before. (in which case, if it wasn't rejected, it may just need someone to take an interest)

Implement FlushToDisk().

eca321a

Fix dotnet#86836

dotnet-issue-labeler bot added area-System.IO new-api-needs-documentation labels Jul 18, 2023

ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 18, 2023

danmoseley reviewed Jul 18, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.cs Outdated Show resolved Hide resolved

danmoseley reviewed Jul 18, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.cs Outdated Show resolved Hide resolved

ericmutta and others added 2 commits July 18, 2023 20:58

Fix typo.

be1e81c

This "platform-depedent" should read "platform-dependent". Co-authored-by: Dan Moseley <danmose@microsoft.com>

Address PR feedback.

32dcdb2

Declarations of class members in System.Runtime.cs should be in alphabetic order (see dotnet#89100 (comment)). Simplify comments on the call to "FileStreamHelpers.FlushToDisk()" (see dotnet#89100 (comment)).

adamsitnik reviewed Jul 19, 2023

View reviewed changes

adamsitnik self-assigned this Jul 19, 2023

danmoseley reviewed Jul 19, 2023

View reviewed changes

src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs Outdated Show resolved Hide resolved

adamsitnik reviewed Jul 21, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.cs Outdated Show resolved Hide resolved

ericmutta added 2 commits July 21, 2023 18:56

danmoseley reviewed Jul 21, 2023

View reviewed changes

src/libraries/System.IO.FileSystem/tests/RandomAccess/FlushToDisk.cs Outdated Show resolved Hide resolved

adamsitnik reviewed Jul 24, 2023

View reviewed changes

src/libraries/System.IO.FileSystem/tests/RandomAccess/FlushToDisk.cs Outdated Show resolved Hide resolved

src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs Outdated Show resolved Hide resolved

ericmutta added 2 commits July 24, 2023 17:54

Avoid randomness in ReadFromDiskAfterFlushToDisk().

8ab5bee

See dotnet#89100 (comment)

Avoid randomness in UpdatesFileLastWriteTime().

54a3215

See the following: - dotnet#89100 (comment) - dotnet#89100 (comment)

adamsitnik reviewed Aug 3, 2023

View reviewed changes

src/libraries/System.IO.FileSystem/tests/RandomAccess/FlushToDisk.Unix.cs Outdated Show resolved Hide resolved

adamsitnik added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 3, 2023

ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 4, 2023

adamsitnik approved these changes Aug 4, 2023

View reviewed changes

This was referenced Aug 4, 2023

Tracking issue for CI build timeouts #76454

Closed

JIT/jit64/opt/rngchk/RngchkStress2.cs failing to build with error CS8078: An expression is too long or complex to compile #87879

Closed

jeffhandley approved these changes Aug 7, 2023

View reviewed changes

adamsitnik merged commit 231b8dc into dotnet:main Aug 7, 2023
164 of 167 checks passed

adamsitnik added this to the 8.0.0 milestone Aug 7, 2023

ericmutta deleted the issue-86836 branch August 7, 2023 19:32

ghost locked as resolved and limited conversation to collaborators Sep 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement RandomAccess.FlushToDisk() #89100

Implement RandomAccess.FlushToDisk() #89100

ericmutta commented Jul 18, 2023

dotnet-issue-labeler bot commented Jul 18, 2023

ghost commented Jul 18, 2023

danmoseley Jul 18, 2023

ericmutta Jul 18, 2023

danmoseley Jul 18, 2023

adamsitnik left a comment

ericmutta commented Jul 19, 2023

danmoseley Jul 19, 2023

ericmutta Jul 25, 2023

danmoseley Jul 19, 2023

ericmutta Jul 25, 2023

ericmutta commented Jul 21, 2023

adamsitnik left a comment

adamsitnik left a comment

adamsitnik left a comment

adamsitnik commented Aug 7, 2023

jeffhandley left a comment

adamsitnik commented Aug 7, 2023

ericmutta commented Aug 7, 2023

danmoseley commented Aug 7, 2023

ericmutta commented Aug 8, 2023

adamsitnik commented Aug 8, 2023

danmoseley commented Aug 10, 2023

Implement RandomAccess.FlushToDisk() #89100

Implement RandomAccess.FlushToDisk() #89100

Conversation

ericmutta commented Jul 18, 2023

dotnet-issue-labeler bot commented Jul 18, 2023

ghost commented Jul 18, 2023

danmoseley Jul 18, 2023

Choose a reason for hiding this comment

ericmutta Jul 18, 2023

Choose a reason for hiding this comment

danmoseley Jul 18, 2023

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

ericmutta commented Jul 19, 2023

danmoseley Jul 19, 2023

Choose a reason for hiding this comment

ericmutta Jul 25, 2023

Choose a reason for hiding this comment

danmoseley Jul 19, 2023

Choose a reason for hiding this comment

ericmutta Jul 25, 2023

Choose a reason for hiding this comment

ericmutta commented Jul 21, 2023

adamsitnik left a comment

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

adamsitnik commented Aug 7, 2023

jeffhandley left a comment

Choose a reason for hiding this comment

adamsitnik commented Aug 7, 2023

ericmutta commented Aug 7, 2023

danmoseley commented Aug 7, 2023

ericmutta commented Aug 8, 2023

adamsitnik commented Aug 8, 2023

danmoseley commented Aug 10, 2023