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

Dont cache file length when handle has been exposed #50424

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,34 @@ public async Task NoDataIsLostWhenWritingToFile(ReadWriteMode mode)
byte[] allBytes = File.ReadAllBytes(filePath);
Assert.Equal(writtenBytes.ToArray(), allBytes);
}

[Theory]
[InlineData(FileAccess.Write)]
[InlineData(FileAccess.ReadWrite)] // FileAccess.Read does not allow for length manipulations
public async Task LengthIsNotCachedAfterHandleHasBeenExposed(FileAccess fileAccess)
{
using FileStream stream = (FileStream)await CreateStream(Array.Empty<byte>(), fileAccess);
using FileStream createdFromHandle = new FileStream(stream.SafeFileHandle, fileAccess);
Copy link
Member

Choose a reason for hiding this comment

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

Do you also need a test that marks it as exposed part way through rather than immediately?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. In this test, we have a FileStream that gets exposed by accessing SafeFileHandle and another one that gets exposed by creating it from the handle. The modification of Length is verified in both directions. IMO it's enough.

If you believe that there is a good reason to do it, please let me know why and I'll be happy to send another PR. In the meantime, I am going to merge it as I really want to start testing other repos against the new implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Because you added code that ignores a _length cache if _exposed is true. For both stream and createFromHandle here, you'll never get to the point where you cache _length in Length, because _exposed is true before Length is ever accessed.


Assert.Equal(0, stream.Length);
Assert.Equal(0, createdFromHandle.Length);

createdFromHandle.SetLength(1);
Assert.Equal(1, createdFromHandle.Length);
Assert.Equal(1, stream.Length);

createdFromHandle.SetLength(2);
Assert.Equal(2, createdFromHandle.Length);
Assert.Equal(2, stream.Length);

stream.SetLength(1);
Assert.Equal(1, stream.Length);
Assert.Equal(1, createdFromHandle.Length);

stream.SetLength(2);
Assert.Equal(2, stream.Length);
Assert.Equal(2, createdFromHandle.Length);
}
}

public class UnbufferedSyncFileStreamStandaloneConformanceTests : FileStreamStandaloneConformanceTests
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
using System.Diagnostics;
using System.Threading.Tasks;
using Microsoft.Win32.SafeHandles;
using System.Runtime.CompilerServices;

namespace System.IO.Strategies
{
Expand All @@ -28,6 +27,7 @@ internal abstract class WindowsFileStreamStrategy : FileStreamStrategy
protected long _filePosition;
private long _appendStart; // When appending, prevent overwriting file.
private long _length = -1; // When the file is locked for writes (_share <= FileShare.Read) cache file length in-memory, negative means that hasn't been fetched.
private bool _exposedHandle; // created from handle, or SafeFileHandle was used and the handle got exposed

internal WindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access, FileShare share)
{
Expand All @@ -37,6 +37,7 @@ internal WindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access, Fil
// but we can't as they're readonly.
_access = access;
_share = share;
_exposedHandle = true;

// As the handle was passed in, we must set the handle field at the very end to
// avoid the finalizer closing the handle when we throw errors.
Expand Down Expand Up @@ -77,15 +78,29 @@ internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access

// When the file is locked for writes we can cache file length in memory
// and avoid subsequent native calls which are expensive.
public unsafe sealed override long Length => _share > FileShare.Read ?
FileStreamHelpers.GetFileLength(_fileHandle, _path) :
_length < 0 ? _length = FileStreamHelpers.GetFileLength(_fileHandle, _path) : _length;
public unsafe sealed override long Length
{
get
{
if (_share > FileShare.Read || _exposedHandle)
{
return FileStreamHelpers.GetFileLength(_fileHandle, _path);
}

if (_length < 0)
{
_length = FileStreamHelpers.GetFileLength(_fileHandle, _path);
}

return _length;
}
}
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved

protected void UpdateLengthOnChangePosition()
{
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
// Do not update the cached length if the file is not locked
// or if the length hasn't been fetched.
if (_share > FileShare.Read || _length < 0)
if (_share > FileShare.Read || _length < 0 || _exposedHandle)
{
Debug.Assert(_length < 0);
return;
Expand Down Expand Up @@ -120,6 +135,10 @@ internal sealed override SafeFileHandle SafeFileHandle
// in memory position is out-of-sync with the actual file position.
FileStreamHelpers.Seek(_fileHandle, _path, _filePosition, SeekOrigin.Begin);
}

_exposedHandle = true;
_length = -1; // invalidate cached length
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved

return _fileHandle;
}
}
Expand Down