-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Introduce FileStreamStrategy as a first step of FileStream rewrite #47128
Merged
Merged
Changes from all commits
Commits
Show all changes
101 commits
Select commit
Hold shift + click to select a range
c0ded00
rename FileStream -> FileStreamImpl FILES
adamsitnik 0fe40d1
rename FileStream -> FileStreamImpl CLASS, make it internal and sealed
adamsitnik e352313
restore orgiginal FileStream
adamsitnik 550e63f
ctors
adamsitnik 62d863c
introduce new abstract class to have Lock, Unlock and Handle methods …
adamsitnik 14d2aa0
implement FlushAsync
adamsitnik 796d7a3
add all virtual methods to the base abstrac class (and rename it)
adamsitnik 2108bd6
implement Read(byte[] buffer, int offset, int count)
adamsitnik 3cc6d07
ReadAsync(byte[] buffer, int offset, int count, CancellationToken can…
adamsitnik 0a202c0
ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = …
adamsitnik c45e7eb
Write(byte[] buffer, int offset, int count)
adamsitnik cedc13a
Write(ReadOnlySpan<byte> buffer)
adamsitnik 4b3d048
WriteAsync(byte[] buffer, int offset, int count, CancellationToken ca…
adamsitnik bafc099
WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellatio…
adamsitnik 91513ed
Flush() and Flush(bool flushToDisk)
adamsitnik ecd62f0
CanRead and CanWrite
adamsitnik 1faf34a
input validation should be always performed in the public type
adamsitnik fe308fe
SetLength(long value)
adamsitnik 082d630
SafeFileHandle, Name, IsAsync and Length
adamsitnik cb6adc3
Position
adamsitnik 2feeefa
IsClosed
adamsitnik 1301318
ReadByte()
adamsitnik 7f15975
WriteByte(byte value)
adamsitnik 6089931
finalizer
adamsitnik d26deb4
BeginRead(byte[] buffer, int offset, int count, AsyncCallback? callba…
adamsitnik 0a9fd33
BeginWrite(byte[] buffer, int offset, int count, AsyncCallback? callb…
adamsitnik f681b6f
EndRead and EndWrite
adamsitnik f880335
FileStream(SafeFileHandle handle, FileAccess access, int bufferSize)
adamsitnik fceeb0d
fix compilation errors
adamsitnik 80fdf68
remove unused fields and methods
adamsitnik e1459f1
CanSeek and Seek
adamsitnik 7cf008d
fix all compilation errors
adamsitnik d1d6528
add missing overrides (implemented in OS-specific files and not caugh…
adamsitnik d30f371
make sure FileStream is not used in FileStreamImpl
adamsitnik 99868c4
rename FileStreamImplBase to FileStreamStrategy and move it to a sepa…
adamsitnik 07ed850
it might seem to have to sense now, but we plan to introduce dedicate…
adamsitnik 19ef1a7
some minor polishing
adamsitnik 50a6850
reference field can be null in finalizer
adamsitnik a0b3196
it looks like having this finalizer is mandatory, as we can not guara…
adamsitnik e3b23fd
fix a typo in the comments
adamsitnik 2581181
rename _actualImplementation to _impl as suggested in code review
adamsitnik 62580e8
move ctor argument validation logic back to FileStream (it's going to…
adamsitnik 82ac0d7
move handle validation logic to FileStream, make _bufferLength readonly
adamsitnik d35bd15
move more validation logic to FileStream
adamsitnik 7a4c761
more polishing
adamsitnik 7c95a6c
reduce code duplication in ctors
adamsitnik 85ab921
the Unix implementation was assuming type check is not needed
adamsitnik ccfea9a
fix the Flush bug that I've introduced
adamsitnik 97a8c51
make sure base.CopyToAsync is called for all custom types that Derive…
adamsitnik a288c2a
introduce DerivedFileStreamImpl, remove GetType checks from FileStream
adamsitnik 662fa2c
all FileStream methods should just delegate the actual logic to Strategy
adamsitnik 233df39
clarify all "base" keyword usages
adamsitnik b4b31c4
clarify all "base" keyword usages, make sure the comments are in the …
adamsitnik 19cec28
Flush() called from strategy.SafeFileHandle should call the virtual F…
adamsitnik 3a2f106
remove the if statement (it's going to become switch)
adamsitnik 4e3796c
get rid of the Impl word, use Strategy everywhere
adamsitnik 227d98d
remove .Handle from the Strategy
adamsitnik 8c865a8
rename FileStreamImpl => CommonFileStreamStrategyTemplate
adamsitnik e82a235
rename FileStreamImpl.Unix => UnixFileStreamStrategy
adamsitnik 8756821
rename FileStreamImpl.Windows => WindowsFileStreamStrategy
adamsitnik d7dd225
make the fields protected so they can be accessed from derived types
adamsitnik 23446ee
remove handle
adamsitnik 4793717
fix finalizer
adamsitnik 6c79ac5
fix the ctors, make init methods abstract and part of the template
adamsitnik cc65678
Lock & Unlock
adamsitnik 6d2db24
FlushWriteBuffer
adamsitnik 01899df
FlushOSBuffer
adamsitnik c2cc411
there is no need to have selead methods in a sealed type...
adamsitnik a92424d
SetLength
adamsitnik e360787
Length {get;}
adamsitnik 3791ecc
OnBufferAllocated
adamsitnik c1a418a
FillReadBufferForReadByte
adamsitnik dab77a3
FlushWriteBufferForWriteByte
adamsitnik 6e8d201
ReadSpan WriteSpan
adamsitnik 08bbb0f
ReadAsyncInternal WriteAsyncInternal
adamsitnik 4e86030
SeekCore
adamsitnik c85c303
make the private helpers protected so they can be accessed from deriv…
adamsitnik 5802b83
replaces usages of FileStreamImpl with appropriate Strategy
adamsitnik 33f58df
remove Handle from DerivedFileStreamStrategy
adamsitnik 2e20ab9
fix the ctors, it compiles on Windows!
adamsitnik 1704b83
this is not needed anymore
adamsitnik 34b7822
fix compilation errors for Unix
adamsitnik 81cd5eb
fix the build?
adamsitnik 0a4c55c
rename CommonFileStreamStrategyTemplate to FileStreamStrategyBase bef…
adamsitnik 01f0cc3
introduce FileStreamStrategyHelper and move some of the parameterless…
adamsitnik 692b965
introduce ChooseStrategy method, remove #ifs
adamsitnik fe202db
fix the Unix build?
adamsitnik 1258ad7
fix the Unix build
adamsitnik d9fc921
rename FileStreamStrategyHelper => FileStreamHelpers
adamsitnik 2d820c3
move all internal Base* methods to the bottom of the source file
adamsitnik 9706f3d
alphabetic order of files in project file
adamsitnik a76ed8c
it looks like the internal FileStream.IsClosed can be removed as we d…
adamsitnik 6204ae5
call virtual Can methods where we used to and still can
adamsitnik ba34492
Merge remote-tracking branch 'upstream/master' into fileStream1stStep
adamsitnik a52f5a8
don't separate Unix and Windows strategies, introduce LegacyFileStrea…
adamsitnik 3a3b3c9
address code review comment and fix last TODO
adamsitnik b41456a
fix Unix build 1/n
adamsitnik 41c3263
update comment
adamsitnik a88f1a5
Update src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs
adamsitnik 5ad9cbb
apply code review suggestions
adamsitnik 835fa17
Apply suggestions from code review
adamsitnik File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
141 changes: 141 additions & 0 deletions
141
src/libraries/System.Private.CoreLib/src/System/IO/DerivedFileStreamStrategy.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,141 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.Win32.SafeHandles; | ||
|
||
namespace System.IO | ||
{ | ||
// this type exists so we can avoid GetType() != typeof(FileStream) checks in FileStream | ||
// when FileStream was supposed to call base.Method() for such cases, we just call _fileStream.BaseMethod() | ||
// for everything else we fall back to the actual strategy (like FileStream does) | ||
// | ||
// it's crucial to NOT use the "base" keyoword here! everything must be using _fileStream or _strategy | ||
internal sealed class DerivedFileStreamStrategy : FileStreamStrategy | ||
{ | ||
private readonly FileStreamStrategy _strategy; | ||
|
||
internal DerivedFileStreamStrategy(FileStream fileStream, FileStreamStrategy strategy) : base(fileStream) => _strategy = strategy; | ||
|
||
public override bool CanRead => _strategy.CanRead; | ||
|
||
public override bool CanWrite => _strategy.CanWrite; | ||
|
||
public override bool CanSeek => _strategy.CanSeek; | ||
|
||
public override long Length => _strategy.Length; | ||
|
||
public override long Position | ||
{ | ||
get => _strategy.Position; | ||
set => _strategy.Position = value; | ||
} | ||
|
||
internal override bool IsAsync => _strategy.IsAsync; | ||
|
||
internal override string Name => _strategy.Name; | ||
|
||
internal override SafeFileHandle SafeFileHandle => _strategy.SafeFileHandle; | ||
|
||
internal override bool IsClosed => _strategy.IsClosed; | ||
|
||
internal override void Lock(long position, long length) => _strategy.Lock(position, length); | ||
|
||
internal override void Unlock(long position, long length) => _strategy.Unlock(position, length); | ||
|
||
public override long Seek(long offset, SeekOrigin origin) => _strategy.Seek(offset, origin); | ||
|
||
public override void SetLength(long value) => _strategy.SetLength(value); | ||
|
||
public override int ReadByte() => _strategy.ReadByte(); | ||
|
||
public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) | ||
=> _strategy.IsAsync | ||
? _strategy.BeginRead(buffer, offset, count, callback, state) | ||
: _fileStream.BaseBeginRead(buffer, offset, count, callback, state); | ||
|
||
public override int EndRead(IAsyncResult asyncResult) | ||
=> _strategy.IsAsync ? _strategy.EndRead(asyncResult) : _fileStream.BaseEndRead(asyncResult); | ||
|
||
public override int Read(byte[] buffer, int offset, int count) => _strategy.Read(buffer, offset, count); | ||
|
||
// If this is a derived type, it may have overridden Read(byte[], int, int) prior to this Read(Span<byte>) | ||
// overload being introduced. In that case, this Read(Span<byte>) overload should use the behavior | ||
// of Read(byte[],int,int) overload. | ||
public override int Read(Span<byte> buffer) | ||
=> _fileStream.BaseRead(buffer); | ||
|
||
// If we have been inherited into a subclass, the Strategy implementation could be incorrect | ||
adamsitnik marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// since it does not call through to Read() which a subclass might have overridden. | ||
// To be safe we will only use this implementation in cases where we know it is safe to do so, | ||
// and delegate to FileStream base class (which will call into Read/ReadAsync) when we are not sure. | ||
public override Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) | ||
=> _fileStream.BaseReadAsync(buffer, offset, count, cancellationToken); | ||
|
||
// If this isn't a concrete FileStream, a derived type may have overridden ReadAsync(byte[],...), | ||
// which was introduced first, so delegate to the base which will delegate to that. | ||
public override ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default) | ||
=> _fileStream.BaseReadAsync(buffer, cancellationToken); | ||
|
||
public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) | ||
=> _strategy.IsAsync | ||
? _strategy.BeginWrite(buffer, offset, count, callback, state) | ||
: _fileStream.BaseBeginWrite(buffer, offset, count, callback, state); | ||
|
||
public override void EndWrite(IAsyncResult asyncResult) | ||
{ | ||
if (_strategy.IsAsync) | ||
{ | ||
_strategy.EndWrite(asyncResult); | ||
} | ||
else | ||
{ | ||
_fileStream.BaseEndWrite(asyncResult); | ||
} | ||
} | ||
|
||
public override void WriteByte(byte value) => _strategy.WriteByte(value); | ||
|
||
public override void Write(byte[] buffer, int offset, int count) => _strategy.Write(buffer, offset, count); | ||
|
||
// If this is a derived type, it may have overridden Write(byte[], int, int) prior to this Write(ReadOnlySpan<byte>) | ||
// overload being introduced. In that case, this Write(ReadOnlySpan<byte>) overload should use the behavior | ||
// of Write(byte[],int,int) overload. | ||
public override void Write(ReadOnlySpan<byte> buffer) | ||
=> _fileStream.BaseWrite(buffer); | ||
|
||
// If we have been inherited into a subclass, the Strategy implementation could be incorrect | ||
// since it does not call through to Write() or WriteAsync() which a subclass might have overridden. | ||
// To be safe we will only use this implementation in cases where we know it is safe to do so, | ||
// and delegate to our base class (which will call into Write/WriteAsync) when we are not sure. | ||
public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) | ||
=> _fileStream.BaseWriteAsync(buffer, offset, count, cancellationToken); | ||
|
||
// If this isn't a concrete FileStream, a derived type may have overridden WriteAsync(byte[],...), | ||
// which was introduced first, so delegate to the base which will delegate to that. | ||
public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken = default) | ||
=> _fileStream.BaseWriteAsync(buffer, cancellationToken); | ||
|
||
public override void Flush() => throw new InvalidOperationException("FileStream should never call this method."); | ||
|
||
internal override void Flush(bool flushToDisk) => _strategy.Flush(flushToDisk); | ||
|
||
// If we have been inherited into a subclass, the following implementation could be incorrect | ||
// since it does not call through to Flush() which a subclass might have overridden. To be safe | ||
// we will only use this implementation in cases where we know it is safe to do so, | ||
// and delegate to our base class (which will call into Flush) when we are not sure. | ||
public override Task FlushAsync(CancellationToken cancellationToken) | ||
=> _fileStream.BaseFlushAsync(cancellationToken); | ||
|
||
// We also need to take this path if this is a derived | ||
// instance from FileStream, as a derived type could have overridden ReadAsync, in which | ||
// case our custom CopyToAsync implementation isn't necessarily correct. | ||
public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) | ||
=> _fileStream.BaseCopyToAsync(destination, bufferSize, cancellationToken); | ||
|
||
public override ValueTask DisposeAsync() => _fileStream.BaseDisposeAsync(); | ||
|
||
internal override void DisposeInternal(bool disposing) => _strategy.DisposeInternal(disposing); | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "Derived" is an odd name here, given that FileStreamStrategy is abstract and any implementation will necessarily be "derived". We often use the term "Delegating" in such cases, e.g.
DelegatingHandler
,DelegatingStream
, etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this particular context, it meant a strategy for a type that derives from FileStream. Does it make more sense or would you like me to rename it to
DelegatingFileStreamStrategy
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So "DerivedFileStreamFileStreamStrategy" :)
I don't have a strong opinion. If you want to stick with DerivedFileStreamStrategy, that's fine.