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

FileStream.FlushAsync ends up doing synchronous writes #27643

Closed
stephentoub opened this issue Oct 16, 2018 · 5 comments · Fixed by #48813
Closed

FileStream.FlushAsync ends up doing synchronous writes #27643

stephentoub opened this issue Oct 16, 2018 · 5 comments · Fixed by #48813
Assignees
Labels
area-System.IO tenet-performance Performance related issue
Milestone

Comments

@stephentoub
Copy link
Member

https://github.com/dotnet/coreclr/blob/85ed652fef5f1dec7c532bd4963dd3cde0199211/src/System.Private.CoreLib/shared/System/IO/FileStream.Windows.cs#L1547-L1561

            // The always synchronous data transfer between the OS and the internal buffer is intentional 
            // because this is needed to allow concurrent async IO requests. Concurrent data transfer
            // between the OS and the internal buffer will result in race conditions. Since FlushWrite and
            // FlushRead modify internal state of the stream and transfer data between the OS and the 
            // internal buffer, they cannot be truly async. We will, however, flush the OS file buffers
            // asynchronously because it doesn't modify any internal state of the stream and is potentially 
            // a long running process.
            try
            {
                FlushInternalBuffer();
            }
            catch (Exception e)
            {
                return Task.FromException(e);
            }

and

        private void FlushInternalBuffer()
        {
            AssertBufferInvariants();
            if (_writePos > 0)
            {
                FlushWriteBuffer();
            }
            else if (_readPos < _readLength && CanSeek)
            {
                FlushReadBuffer();
            }
        }

This is particularly problematic when the FileStream is configured to use async I/O on Windows, as then FlushWriteBuffer needs to use sync-over-async to do the write, queueing the overlapped I/O and then blocking waiting for the operation to complete... so we have the asynchronous FlushAsync synchronously blocking waiting for the overlapped I/O to complete. Ugh.

Related to https://github.com/dotnet/corefx/issues/31914#issuecomment-427120612
Related to https://github.com/dotnet/corefx/issues/6007
Related to https://github.com/dotnet/corefx/issues/6039
Related to https://github.com/dotnet/corefx/issues/29129
Related to https://github.com/dotnet/corefx/issues/27226
cc: @JeremyKuhne, @danmosemsft

@benaadams
Copy link
Member

This is a troubling fix NuGet/NuGet.Client#2488 😢

@stephentoub
Copy link
Member Author

stephentoub commented Oct 19, 2018

This is a troubling fix

That is actually the fix/workaround I've recommended: setting useAsync:false. As far as I can tell, there's little benefit in that scenario for using overlapped I/O, and even if FileStream were perfect, there's overhead to overlapped I/O.

@benaadams
Copy link
Member

Yeah, don't mean its not the right fix for usage; just mean FileStream needs some TLC for async.

Though when I tried it before, I did find it was quite complicated to improve...

@stephentoub
Copy link
Member Author

just mean FileStream needs some TLC for async

It definitely does. :)

@danmoseley
Copy link
Member

This cannot fit in 3.0. It's on the radar for v next.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@stephentoub stephentoub removed the untriaged New issue has not been triaged by the area owner label Feb 25, 2020
@carlossanlop carlossanlop modified the milestones: 5.0.0, Future Jun 18, 2020
@adamsitnik adamsitnik self-assigned this Mar 1, 2021
@adamsitnik adamsitnik modified the milestones: Future, 6.0.0 Mar 1, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 1, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 17, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants