Win32 FileStream turns async reads into sync reads

[stephentoub]

When filling the internal buffer, Win32FileStream does this as part of ReadAsync:

Task<int> readTask = ReadInternalCoreAsync(_buffer, 0, _bufferSize, 0, cancellationToken);
_readLen = readTask.GetAwaiter().GetResult();

Ugh!

There's a large comment about how this is done to avoid concurrent use of the buffer when concurrent read operations are issued, but we should be able to work around that using something similar to what I previously did for WriteAsync and FlushAsync, with the HasActiveBufferOperation mechanism (dotnet/corefx#2929).

As it currently stands, it appears that when async reads are performed against a Win32FileStream and those reads are smaller than the file stream's buffer, all such reads will be made synchronous, either because they're pulling from the buffer or because they're blocking waiting for the buffer to be filled.

cc: @ericstj, @ianhays, @JeremyKuhne

[stephentoub]

I spent a day on this and found it's going to be more complicated than I'd initially thought. It's going to need a large overhaul of the code base as well as likely compromising on some of the behavioral details of the type. In particular, there are assumptions throughout the code that reads using the buffer are always going to be synchronous such that code to synchronous methods like SetLength and ReadByte and whatnot can rely on the current values of buffer-related state, like _readPos. I think the basic approach will be:

• Use _activeBufferOperation to track whether there's currently any operation using the buffer and its related state
• Update async reads and writes such that if there isn't an active operation, the call is immediately invoked and _activeBufferOperation is set to the resulting task for the whole operation or whichever part of it actually uses the buffer and its state, and if there is an active operation, make the new call the continuation of whatever was tracked by _activeBufferOperation
• Update all synchronous operations to appropriately delegate to async counterparts if there is an active buffered operation
• Determine how we want to handle the fact that today you can rely on the FileStream's position (both tracked by FileStream and in the handle) after these async operations return to the synchronous caller, and how that'll be impacted by multiple outstanding async operations that may be continuations and thus may not have actually updated the state yet.

[stephentoub]

@danmosemsft, @JeremyKuhne, @pjanotti, any chance we can get this fixed for 2.1? It's pretty nasty.

[danmoseley]

So far as I can tell this goes back roughly to when async support was added, in 2010. To help prioritize, do we have data on the impact, and any customer reports of it?

[stephentoub]

The impact is async isn't async. I don't have concrete data other than "ugh" :)

[danmoseley]

OK, let's consider it when @JeremyKuhne completes file enumeration work. We know that wont be the last IO win to make.

[benaadams]

To help prioritize, do we have data on the impact, and any customer reports of it?

Kestrel is much faster at serving application content than IIS; but much slower at serving file content (which use the .NET file apis), it would be good to redress this balance. This is likely to be one of the many links in the chain.

[danmoseley]

@benaadams thanks. Do you see indications in profiles of your own server that the issue might be here? I guess it would look like high inclusive time in ReadAsyncInternal

[benaadams]

An async function calling an async function then performing a sync wait on it is just trollin', surely?

I'll see if I can get some data :)

[danmoseley]

Yeah, it's obviously not right

[danmoseley]

Did you happen to end up getting data @benadams? Totally fine if you didn't.

As it is this isn't likely to happen for 2.1.

[benaadams]

No sorry, haven't had time

[danmoseley]

OK !

[benaadams]

O_o

Read and Write share a buffer, so if there is anything in the buffer for Write it needs to flush it first. To do so it queues a Flush (which it also does in other places):

// Handle buffering.
if (_writePos > 0) FlushWriteBuffer();
if (_readPos == _readLength)
{
    if (destination.Length < _bufferLength)
    {
        Task<int> readTask = ReadNativeAsync(new Memory<byte>(GetBuffer()), 0, cancellationToken);
        _readLength = readTask.GetAwaiter().GetResult();

FlushWriteBuffer() to ensure its completed also does synchronous blocking

Task writeTask = FlushWriteAsync(CancellationToken.None);
if (!calledFromFinalizer)
{
    writeTask.GetAwaiter().GetResult();
}

[jasonmalinowski]

I ran across our large comment in Roslyn that points to this bug and was curious to see what ever came of this. Roslyn still has a pretty fun workaround for this:

https://github.com/dotnet/roslyn/blob/0dc28dc46c494189d1aca48224fa51eca548e8bd/src/Workspaces/Core/Portable/Workspace/FileTextLoader.cs#L106-L171

The impact is async isn't async. I don't have concrete data other than "ugh" :)

When we originally discovered this, it was because a customer had Visual Studio hanging for minutes at a time while the thread pool got starved. We had to ship a private hotfix for that customer with that workaround. So at the impact was worse than just "ugh". We're past it and haven't been bitten since so I'm not pushing for this to prioritized, but just wanted to illustrate a fun case where the impact was pretty bad. 😄