A bug in FileStream (async + buffer) since .Net 6?

Description

I encounter a strange issue in FileStream. I provide the code and test data in https://github.com/yeunglee/file_stream_test

The major issue is that (async random read + special buffer size + special file) in FileStream encounters unexpected read.

Here is a simple code. The full code can be retrieved from https://github.com/yeunglee/file_stream_test

const string FILE_PATH = "test_data";

// SOLUTION #1: change buffer size. for example: 1024, 4096, 1024 * 512 - 1, 1024 * 512 + 1
const int FILE_BUFFER_SIZE = 1024 * 512;

Console.WriteLine($".Net version: {Environment.Version}");
Console.WriteLine($"OS version: {Environment.OSVersion}");

var fileData = await File.ReadAllBytesAsync(FILE_PATH);
// SOLUTION #2: add FileOptions.RandomAccess
await using var fs = new FileStream(FILE_PATH, FileMode.Open, FileAccess.Read, FileShare.ReadWrite, FILE_BUFFER_SIZE, FileOptions.Asynchronous);

void SetPosition(long p) => fs.Position = p;

async Task ReadAsync(int size)
{
    var initialPosition = fs.Position;
    var content = new byte[size];

    // SOLUTION #3: use sync Read instead of async ReadAsync
    var count = await fs.ReadAsync(content, 0, content.Length);

    var isMatched = fileData.Skip((int)initialPosition).Take(count).SequenceEqual(content.Take(count));

    if (!isMatched)
    {
        Console.WriteLine($"!!!Content is inconsistent!!! position={initialPosition:#,0} size={size:#,0} count={count:#,0}");
        Environment.Exit(1);
    }
}

await ReadAsync(1);
await ReadAsync(4);
await ReadAsync(4);
await ReadAsync(118);
await ReadAsync(4);
await ReadAsync(524288);
SetPosition(524377);
await ReadAsync(524288);
SetPosition(1048633);
await ReadAsync(524288);
SetPosition(1572864);
await ReadAsync(524288);
await ReadAsync(524288);
SetPosition(2621431);
await ReadAsync(524288);

Reproduction Steps

Checkout the demo code from https://github.com/yeunglee/file_stream_test
Execute dotnet run (The dotnet project file specify .Net 6, you can change .Net version)
Check the execution result from step#2.

Expected behavior

The demo program outputs:
FileStream is good

Actual behavior

The demo program outputs:
!!!Content is inconsistent!!! position=2,621,431 size=524,288 count=326,069

Regression?

.Net 3.1 and .Net 5 are fine.

Known Workarounds

There are ~~three~~ four workarounds:

Change file buffer size
Or change file option (add FileOptions.RandomAccess)
Or use sync Read instead of async ReadAsync

The workaround ways are commented in demo program code.

The demo code is fine with .Net 5, .Net 3.1

Configuration

This issue can be reproduced in

WinServer 2022 (21H2) (x64)+ .Net 6.0.3
Win11 Enterprise (22H2) (x64) + .Net 7.0.1
Win11 Enterprise (22H2) (x64) + .Net 6.0.12
Win10 LTSC (1809) (x64) + .Net 6.0.11

Both Debug and Release configuration are same result.

Other information

No response

@adamsitnik

@adamsitnik

CAUSE

This bug is caused by

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs

Lines 319 to 324 in dcda1e0

    
           if (_readLen == _readPos && buffer.Length >= _bufferSize) 
        
           { 
        
               // hot path #1: the read buffer is empty and buffering would not be beneficial 
        
               // To find out why we are bypassing cache here, please see WriteAsync comments. 
        
               return _strategy.ReadAsync(buffer, cancellationToken); 
        
           }

And

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs

Lines 914 to 935 in dcda1e0

    
           long oldPos = Position; 
        
           Debug.Assert(oldPos == _strategy.Position + (_readPos - _readLen)); 
        
           long newPos = _strategy.Seek(offset, origin); 
        
           // If the seek destination is still within the data currently in the buffer, we want to keep the buffer data and continue using it. 
        
           // Otherwise we will throw away the buffer. This can only happen on read, as we flushed write data above. 
        
           // The offset of the new/updated seek pointer within _buffer: 
        
           long readPos = (newPos - (oldPos - _readPos)); 
        
           // If the offset of the updated seek pointer in the buffer is still legal, then we can keep using the buffer: 
        
           if (0 <= readPos && readPos < _readLen) 
        
           { 
        
               _readPos = (int)readPos; 
        
               // Adjust the seek pointer of the underlying stream to reflect the amount of useful bytes in the read buffer: 
        
               _strategy.Seek(_readLen - _readPos, SeekOrigin.Current); 
        
           } 
        
           else 
        
           {  // The offset of the updated seek pointer is not a legal offset. Loose the buffer. 
        
               _readPos = _readLen = 0; 
        
           }

FIX

The fix is pretty easy. Just make the read buffer unavailable. Like what it does:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs

Lines 426 to 439 in dcda1e0

    
           _readPos = _readLen = 0; 
        
           // If there was anything in the write buffer, clear it. 
        
           if (_writePos > 0) 
        
           { 
        
               await _strategy.WriteAsync(new ReadOnlyMemory<byte>(_buffer, 0, _writePos), cancellationToken).ConfigureAwait(false); 
        
               _writePos = 0; 
        
           } 
        
           // If the requested read is larger than buffer size, avoid the buffer and still use a single read: 
        
           if (buffer.Length >= _bufferSize) 
        
           { 
        
               return await _strategy.ReadAsync(buffer, cancellationToken).ConfigureAwait(false) + bytesAlreadySatisfied; 
        
           }

e.g. Add _readLen = _readPos = 0; and write flush if need before return _strategy.ReadAsync(buffer, cancellationToken); (Lines 319 to 324)

QUESITON

But I don't understand the comment:

To find out why we are bypassing cache here, please see WriteAsync comments.

Any idea?

@yeunglee

@yeunglee big thanks for a very detailed bug report, I've sent a fix in #80432

But I don't understand the comment:

The comment refers to the following comment, which explains why we have such perf optimization:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs

Lines 617 to 630 in 7a6c31c

    
           // hot path #1: the write buffer is empty and buffering would not be beneficial 
        
           if (_writePos == 0 && buffer.Length >= _bufferSize) 
        
           { 
        
               // The fact that Strategy can be wrapped by BufferedFileStreamStrategy 
        
               // is transparent to every Strategy implementation. It means, that 
        
               // every Strategy must work fine no matter if buffering is enabled or not. 
        
               // In case of AsyncWindowsFileStreamStrategy.WriteAsync, 
        
               // it updates it's private position BEFORE it enqueues the IO request. 
        
               // This combined with the fact that BufferedFileStreamStrategy state 
        
               // is not modified here, allows us to NOT await the call 
        
               // and release the lock BEFORE the IO request completes. 
        
               // It improves the performance of common scenario, where buffering is enabled (default) 
        
               // but the user provides buffers larger (or equal) to the internal buffer size. 
        
               return _strategy.WriteAsync(buffer, cancellationToken);

@yeunglee

@yeunglee big thanks for a very detailed bug report, I've sent a fix in #80432

But I don't understand the comment:

The comment refers to the following comment, which explains why we have such perf optimization:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs

Lines 617 to 630 in 7a6c31c

// hot path #1: the write buffer is empty and buffering would not be beneficial

if (_writePos == 0 && buffer.Length >= _bufferSize)

{

// The fact that Strategy can be wrapped by BufferedFileStreamStrategy

// is transparent to every Strategy implementation. It means, that

// every Strategy must work fine no matter if buffering is enabled or not.

// In case of AsyncWindowsFileStreamStrategy.WriteAsync,

// it updates it's private position BEFORE it enqueues the IO request.

// This combined with the fact that BufferedFileStreamStrategy state

// is not modified here, allows us to NOT await the call

// and release the lock BEFORE the IO request completes.

// It improves the performance of common scenario, where buffering is enabled (default)

// but the user provides buffers larger (or equal) to the internal buffer size.

return _strategy.WriteAsync(buffer, cancellationToken);

You're welcome. I posted a wrong comment. I have already deleted it.

Navigation Menu

Repository navigation

A bug in FileStream (async + buffer) since .Net 6? #80344

Description

Reproduction Steps

Expected behavior

Actual behavior

Regression?

Known Workarounds

Configuration

Other information

CAUSE

FIX

QUESITON

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	if (_readLen == _readPos && buffer.Length >= _bufferSize)
	{
	// hot path #1: the read buffer is empty and buffering would not be beneficial
	// To find out why we are bypassing cache here, please see WriteAsync comments.
	return _strategy.ReadAsync(buffer, cancellationToken);
	}

	long oldPos = Position;
	Debug.Assert(oldPos == _strategy.Position + (_readPos - _readLen));

	long newPos = _strategy.Seek(offset, origin);

	// If the seek destination is still within the data currently in the buffer, we want to keep the buffer data and continue using it.
	// Otherwise we will throw away the buffer. This can only happen on read, as we flushed write data above.

	// The offset of the new/updated seek pointer within _buffer:
	long readPos = (newPos - (oldPos - _readPos));

	// If the offset of the updated seek pointer in the buffer is still legal, then we can keep using the buffer:
	if (0 <= readPos && readPos < _readLen)
	{
	_readPos = (int)readPos;
	// Adjust the seek pointer of the underlying stream to reflect the amount of useful bytes in the read buffer:
	_strategy.Seek(_readLen - _readPos, SeekOrigin.Current);
	}
	else
	{ // The offset of the updated seek pointer is not a legal offset. Loose the buffer.
	_readPos = _readLen = 0;
	}

	_readPos = _readLen = 0;

	// If there was anything in the write buffer, clear it.
	if (_writePos > 0)
	{
	await _strategy.WriteAsync(new ReadOnlyMemory<byte>(_buffer, 0, _writePos), cancellationToken).ConfigureAwait(false);
	_writePos = 0;
	}

	// If the requested read is larger than buffer size, avoid the buffer and still use a single read:
	if (buffer.Length >= _bufferSize)
	{
	return await _strategy.ReadAsync(buffer, cancellationToken).ConfigureAwait(false) + bytesAlreadySatisfied;
	}

	// hot path #1: the write buffer is empty and buffering would not be beneficial
	if (_writePos == 0 && buffer.Length >= _bufferSize)
	{
	// The fact that Strategy can be wrapped by BufferedFileStreamStrategy
	// is transparent to every Strategy implementation. It means, that
	// every Strategy must work fine no matter if buffering is enabled or not.
	// In case of AsyncWindowsFileStreamStrategy.WriteAsync,
	// it updates it's private position BEFORE it enqueues the IO request.
	// This combined with the fact that BufferedFileStreamStrategy state
	// is not modified here, allows us to NOT await the call
	// and release the lock BEFORE the IO request completes.
	// It improves the performance of common scenario, where buffering is enabled (default)
	// but the user provides buffers larger (or equal) to the internal buffer size.
	return _strategy.WriteAsync(buffer, cancellationToken);

A bug in FileStream (async + buffer) since .Net 6? #80344

Description

Description

Reproduction Steps

Expected behavior

Actual behavior

Regression?

Known Workarounds

Configuration

Other information

Activity

yeunglee commented on Jan 9, 2023

CAUSE

FIX

QUESITON

adamsitnik commented on Jan 10, 2023

yeunglee commented on Jan 10, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions