Skip to content
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

Ensure FileStream.Position is correct after a failed|cancelled WriteAsync attempt #56716

Merged
merged 7 commits into from
Aug 5, 2021

Conversation

adamsitnik
Copy link
Member

@stephentoub I've realised that while writes can't be incomplete, they might fail with an exception and in such cases we should update the position as well.

@adamsitnik adamsitnik added this to the 6.0.0 milestone Aug 2, 2021
@adamsitnik adamsitnik requested a review from stephentoub August 2, 2021 11:55
@ghost
Copy link

ghost commented Aug 2, 2021

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

Issue Details

@stephentoub I've realised that while writes can't be incomplete, they might fail with an exception and in such cases we should update the position as well.

Author: adamsitnik
Assignees: -
Labels:

area-System.IO

Milestone: 6.0.0

@stephentoub
Copy link
Member

stephentoub commented Aug 2, 2021

I've realised that while writes can't be incomplete, they might fail with an exception and in such cases we should update the position as well.

Just to clarify, this never used to be done in previous .NET releases, either, right?

@adamsitnik
Copy link
Member Author

adamsitnik commented Aug 2, 2021

Just to clarify, this never used to be done in previous .NET releases, either, right?

I think it was since .NET Core 3.1 (as least to some degree):

using System;
using System.IO;
using System.Threading;
using System.Threading.Tasks;

namespace fileCancel
{
    class Program
    {
        static async Task Main()
        {
            const int writeSize = 1024 * 1024 * 1024;
            byte[] buffer = new byte[writeSize];
            using (FileStream fs = new FileStream("test.test", FileMode.CreateNew, FileAccess.Write, FileShare.None, bufferSize: 1, useAsync: true))
            {
                CancellationTokenSource cts = new CancellationTokenSource();
                cts.Cancel(); // cancelled BEOFRE operation started
                Task writeTask = fs.WriteAsync(buffer, 0, buffer.Length, cts.Token);
                await AwaitAndSee(writeTask, fs);

                cts = new CancellationTokenSource();
                writeTask = fs.WriteAsync(buffer, 0, buffer.Length, cts.Token);
                cts.Cancel(); // immediately cancelled AFTER operation started
                await AwaitAndSee(writeTask, fs);

                cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(0.2)); // very short timeout
                writeTask = fs.WriteAsync(buffer, 0, buffer.Length, cts.Token);
                await AwaitAndSee(writeTask, fs);
            }

            File.Delete("test.test");
        }

        private static async Task AwaitAndSee(Task task, FileStream fs)
        {
            try
            {
                await task;

                Console.WriteLine($"Not cancelled, position: {fs.Position}");
            }
            catch (OperationCanceledException)
            {
                Console.WriteLine($"Got cancelled, position: {fs.Position}");
            }
        }
    }
}
<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFrameworks>net6.0;net5.0;netcoreapp3.1;netcoreapp2.1;net48</TargetFrameworks>
  </PropertyGroup>

</Project>
PS C:\Projects\repros\fileCancel> dotnet run -c Release -f net48
Got cancelled, position: 0
Not cancelled, position: 1073741824
Not cancelled, position: 2147483648
PS C:\Projects\repros\fileCancel> dotnet run -c Release -f netcoreapp2.1
Got cancelled, position: 0
Not cancelled, position: 1073741824
Not cancelled, position: 2147483648
PS C:\Projects\repros\fileCancel> dotnet run -c Release -f netcoreapp3.1
Got cancelled, position: 0
Not cancelled, position: 1073741824
Got cancelled, position: 1073741824
PS C:\Projects\repros\fileCancel> dotnet run -c Release -f net5.0
Got cancelled, position: 0
Not cancelled, position: 1073741824
Got cancelled, position: 1073741824

@stephentoub
Copy link
Member

stephentoub commented Aug 2, 2021

I think it was since .NET Core 3.1 (as least to some degree):

Isn't your example only showing it actually happening when the token is canceled in advance, which means the operation never actually starts, with our cancellation prechecks causing the position to not be updated in the first place?

Or, if I'm misunderstanding the example, can you point to what code was updating the position after a failure?

@adamsitnik
Copy link
Member Author

@stephentoub you are right, so far we were not updating the position after failure

FWIW I've checked why in my sample we fail to cancel to WriteFile request. The code calls CancelIoEx using proper arguments:

But it fails with ERROR_NOT_FOUND , which we have been always ignoring:

// ERROR_NOT_FOUND is returned if CancelIoEx cannot find the request to cancel.
// This probably means that the IO operation has completed.

@adamsitnik
Copy link
Member Author

Closing an re-opening to try to trigger the CI as the last x64 debug Windows build failed with:

Failed to download package 'System.Reflection.Metadata.1.8.0'

full log

@adamsitnik adamsitnik closed this Aug 3, 2021
@adamsitnik adamsitnik reopened this Aug 3, 2021
@adamsitnik adamsitnik requested a review from stephentoub August 4, 2021 20:27
@adamsitnik
Copy link
Member Author

@stephentoub could you PTAL?

@adamsitnik adamsitnik merged commit 6fc8441 into dotnet:main Aug 5, 2021
@adamsitnik adamsitnik deleted the writeAsyncExceptionalCodePath branch August 5, 2021 05:56
@ghost ghost locked as resolved and limited conversation to collaborators Sep 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants