Fix WriteAsyncCancelledFile test failure

[adamsitnik]

Try to fix #56894

Please ignore this PR until the CI is 100% green

[ghost]

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

Issue Details

---

Try to fix #56894

Please ignore this PR until the CI is 100% green

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

[adamsitnik]

@jozkee some explanation: in #56716 I've changed the code to use Interlocked.Add to update the position in WriteAsync. This particular test is using multiple (10) tasks, so most probably the lack of Interlocked.Read was causing to read an outdated value on x86. I am just guessing since I was not able to repro it locally (in the CI it failed only for WIn 7 x86)

[stephentoub]

What kind of performance impact might this end up having, in particular on 32-bit? Is this just a case of a test doing something unsupported, or we think this access pattern driving the need for this is valid?

[adamsitnik]

What kind of performance impact might this end up having, in particular on 32-bit?

For the following micro-benchmark:

[Benchmark]
[Arguments(OneKibibyte, FileOptions.None)]
[Arguments(OneKibibyte, FileOptions.Asynchronous)]
public void PositionGetSet(long fileSize, FileOptions options)
{
    string filePath = _sourceFilePaths[fileSize];
    using (FileStream fileStream = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.Read, FourKibibytes, options))
    {
        for (long i = 0; fileStream.Position < fileSize; i++)
        {
            fileStream.Position = i;
        }
    }
}

For x86:

Method	Job	fileSize	options	Mean
PositionGetSet	.NET 5	1024	None	730.0 us
PositionGetSet	.NET 6	1024	None	69.95 us
PositionGetSet	this PR	1024	None	329.94 us
PositionGetSet	.NET 5	1024	Asynchronous	2,821.5 us
PositionGetSet	.NET 6	1024	Asynchronous	80.85 us
PositionGetSet	this PR	1024	Asynchronous	346.32 us

So the impact on x86 is quite huge, however, we are still few times faster than .NET 5 and I don't expect this method to be on a hot path.

Is this just a case of a test doing something unsupported, or we think this access pattern driving the need for this is valid?

The test is not doing anything unsupported. It starts an async write operation and tries to cancel it, if the cancellation succeeds, we check for file length and position to see that they are 0.

Based on my analysis of the source code and handling of WriteFile return values and callbacks my only idea was false sharing and the extra interlocked seems to solve the problem.

I am going to see if marking _position as volatile would help.

[adamsitnik]

a volatile field cannot be of the type 'long'

So I don't have other ideas except of disabling this test for x86

[adamsitnik]

Actually I have one more idea, but let's wait to see CI results before we get into details (it might be not worth it)

[jozkee]

How can this have an outdated value? The failing test (WriteAsyncCancelledFile) is creating a new file and a new FileStream instance in each run, right?

[adamsitnik]

It might be caused by false sharing, but I am just guessing: https://docs.microsoft.com/en-us/archive/msdn-magazine/2008/october/net-matters-false-sharing

[stephentoub]

Wow, that's a good article. The lead author must really know what he's talking about. 😄

That said, this isn't false sharing. @jozkee is correct that this value shouldn't be outdated, as it comes linearly after the operations should have completed and quiesced with appropriate synchronization to ensure that all updates were published. I don't know why it's failing, nor why this fixes it, but it suggests a real problem with something potentially being done in a problematic order in the code, such as accidentally changing _filePosition after completing the task.

(When I previously commented, I hadn't actually looked at the test in question. I thought this was trying to fix a problem where the test was accessing position concurrently with asynchronous operations being in flight, in which case there could be torn reads/writes of a 64-bit value on a 32-bit platform. But that doesn't appear to be what this test is doing. Maybe something in the implementation itself is doing so?)

[adamsitnik]

I am going to close this PR and send a new one with the fix once I figure out what exactly is going on. Thank you all for feedback!