Skip to content

Conversation

@adamsitnik
Copy link
Member

private ReadSpan and WriteSpan methods of the BufferedFileStreamStrategy accept both a Span and an ArraySegment for reasons described here:

// For async file stream strategies the call to Read(Span) is translated to Stream.Read(Span),
// which rents an array from the pool, copies the data, and then calls Read(Array). This is expensive!
// To avoid that (and code duplication), the Read(Array) method passes ArraySegment to this method
// which allows for calling Strategy.Read(Array) instead of Strategy.Read(Span).

The bug that I've discovered while working on #50166 was that the Span was getting sliced, while ArraySegment was not. This was causing a serious bug (but the new strategy was not enabled yet so we are safe):

{
source.Slice(0, numBytes).CopyTo(_buffer!.AsSpan(_writePos));
_writePos += numBytes;
source = source.Slice(numBytes);
}

Read is free of this bug as the ArraySegment is properly sliced using n:

int moreBytesRead = arraySegment.Array != null
? _strategy.Read(arraySegment.Array, arraySegment.Offset + n, arraySegment.Count - n)
: _strategy.Read(destination.Slice(n));

I've added a failing tests to the FileStream conformance tests:

[xUnit.net 00:00:08.68]     System.IO.Tests.BufferedAsyncFileStreamStandaloneConformanceTests.NoDataIsLostWhenWritingToFile(mode: SyncArray) [FAIL]
  Failed System.IO.Tests.BufferedAsyncFileStreamStandaloneConformanceTests.NoDataIsLostWhenWritingToFile(mode: SyncArray) [13 ms]
  Error Message:
   Assert.Equal() Failure
Expected: 51
Actual:   52
  Stack Trace:
     at System.IO.Tests.FileStreamStandaloneConformanceTests.NoDataIsLostWhenWritingToFile(ReadWriteMode mode) in C:\Projects\runtime\src\libraries\System.IO.FileSystem\tests\FileStream\FileStreamConformanceTests.cs:line 128
--- End of stack trace from previous location ---
[xUnit.net 00:00:08.76]     System.IO.Tests.BufferedSyncFileStreamStandaloneConformanceTests.NoDataIsLostWhenWritingToFile(mode: SyncArray) [FAIL]
  Failed System.IO.Tests.BufferedSyncFileStreamStandaloneConformanceTests.NoDataIsLostWhenWritingToFile(mode: SyncArray) [3 ms]
  Error Message:
   Assert.Equal() Failure
Expected: 51
Actual:   52
  Stack Trace:
     at System.IO.Tests.FileStreamStandaloneConformanceTests.NoDataIsLostWhenWritingToFile(ReadWriteMode mode) in C:\Projects\runtime\src\libraries\System.IO.FileSystem\tests\FileStream\FileStreamConformanceTests.cs:line 128
--- End of stack trace from previous location ---

…BufferedFileStreamStrategy.cs

Co-authored-by: Stephen Toub <stoub@microsoft.com>
Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, assuming all tests pass.

A Mono CI leg failed with an unrelated error, I re-ran it, hoping it will pass this time:

##[error]Artifact CoreCLRProduct___OSX_x64_release not found for build 1058275. Please ensure you have published artifacts in any previous phases of the current build.

@adamsitnik adamsitnik merged commit db65d37 into dotnet:main Mar 26, 2021
@adamsitnik adamsitnik deleted the fixBufferingBug branch March 26, 2021 18:57
@ghost ghost locked as resolved and limited conversation to collaborators Apr 25, 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.

3 participants