Skip to content

Commit

Permalink
don't try to cache file length when file is opened for writing, as up…
Browse files Browse the repository at this point in the history
…dating file position before performing async write can lead to invalid cached length value
  • Loading branch information
adamsitnik committed Jul 30, 2021
1 parent 079ed8b commit 30b5768
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 49 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -60,20 +60,12 @@ public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationTo

private ValueTask WriteAsyncInternal(ReadOnlyMemory<byte> source, CancellationToken cancellationToken)
{
long positionBefore = _filePosition;
if (CanSeek)
{
// When using overlapped IO, the OS is not supposed to
// touch the file pointer location at all. We will adjust it
// ourselves, but only in memory. This isn't threadsafe.
_filePosition += source.Length;
UpdateLengthOnChangePosition();
}
long writeOffset = CanSeek ? Interlocked.Add(ref _filePosition, source.Length) - source.Length : -1;

(SafeFileHandle.OverlappedValueTaskSource? vts, int errorCode) = RandomAccess.QueueAsyncWriteFile(_fileHandle, source, positionBefore, cancellationToken);
(SafeFileHandle.OverlappedValueTaskSource? vts, int errorCode) = RandomAccess.QueueAsyncWriteFile(_fileHandle, source, writeOffset, cancellationToken);
return vts != null
? new ValueTask(vts, vts.Version)
: (errorCode == 0) ? ValueTask.CompletedTask : ValueTask.FromException(HandleIOError(positionBefore, errorCode));
: (errorCode == 0) ? ValueTask.CompletedTask : ValueTask.FromException(HandleIOError(writeOffset, errorCode));
}

private Exception HandleIOError(long positionBefore, int errorCode)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ internal abstract class OSFileStreamStrategy : FileStreamStrategy
private readonly FileAccess _access; // What file was opened for.

protected long _filePosition;
protected long _length = -1; // negative means that hasn't been fetched.
private long _appendStart; // When appending, prevent overwriting file.
protected long _length = -1; // When the file is locked for writes on Windows ((share & FileShare.Write) == 0) cache file length in-memory, negative means that hasn't been fetched.
private bool _lengthCanBeCached; // SafeFileHandle hasn't been exposed and FileShare.Write was not specified when the handle was opened.
private bool _lengthCanBeCached; // SafeFileHandle hasn't been exposed, file has been opened for reading and not shared for writing.

internal OSFileStreamStrategy(SafeFileHandle handle, FileAccess access)
{
Expand All @@ -44,7 +44,7 @@ internal OSFileStreamStrategy(string path, FileMode mode, FileAccess access, Fil
string fullPath = Path.GetFullPath(path);

_access = access;
_lengthCanBeCached = (share & FileShare.Write) == 0;
_lengthCanBeCached = (share & FileShare.Write) == 0 && (access & FileAccess.Write) == 0;

_fileHandle = SafeFileHandle.Open(fullPath, mode, access, share, options, preallocationSize);

Expand Down Expand Up @@ -100,22 +100,6 @@ public unsafe sealed override long Length
// at the same time. That is why we are using Interlocked here.
internal void OnIncompleteRead(int expectedBytesRead, int actualBytesRead) => Interlocked.Add(ref _filePosition, actualBytesRead - expectedBytesRead);

protected void UpdateLengthOnChangePosition()
{
// Do not update the cached length if the file is not locked
// or if the length hasn't been fetched.
if (!LengthCachingSupported || _length < 0)
{
Debug.Assert(_length < 0);
return;
}

if (_filePosition > _length)
{
_length = _filePosition;
}
}

protected bool LengthCachingSupported => OperatingSystem.IsWindows() && _lengthCanBeCached;

/// <summary>Gets or sets the position within the current stream</summary>
Expand Down Expand Up @@ -291,18 +275,8 @@ public sealed override void Write(ReadOnlySpan<byte> buffer)
ThrowHelper.ThrowNotSupportedException_UnwritableStream();
}

try
{
RandomAccess.WriteAtOffset(_fileHandle, buffer, _filePosition);
}
catch
{
_length = -1; // invalidate cached length
throw;
}

RandomAccess.WriteAtOffset(_fileHandle, buffer, _filePosition);
_filePosition += buffer.Length;
UpdateLengthOnChangePosition();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,8 @@ public override Task WriteAsync(byte[] buffer, int offset, int count, Cancellati

public override ValueTask WriteAsync(ReadOnlyMemory<byte> source, CancellationToken cancellationToken)
{
long filePositionBefore = -1;
if (CanSeek)
{
filePositionBefore = _filePosition;
_filePosition += source.Length;
}

return RandomAccess.WriteAtOffsetAsync(_fileHandle, source, filePositionBefore, cancellationToken);
long writeOffset = CanSeek ? Interlocked.Add(ref _filePosition, source.Length) - source.Length : -1;
return RandomAccess.WriteAtOffsetAsync(_fileHandle, source, writeOffset, cancellationToken);
}

/// <summary>Provides a reusable ValueTask-backing object for implementing ReadAsync.</summary>
Expand Down

0 comments on commit 30b5768

Please sign in to comment.