-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Make SubReadStream seekable when underlying stream is seekable #118720
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
base: main
Are you sure you want to change the base?
Changes from 8 commits
37ef1ed
044fecc
a6c91cf
e855969
d24e917
4b0313b
7153b8d
d0aa6e4
e525dfb
dfd29b2
b559fc0
9d563a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -259,13 +259,20 @@ public override long Position | |
{ | ||
ThrowIfDisposed(); | ||
|
||
throw new NotSupportedException(SR.SeekingNotSupported); | ||
if (!CanSeek) | ||
{ | ||
throw new NotSupportedException(SR.SeekingNotSupported); | ||
} | ||
alinpahontu2912 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
ArgumentOutOfRangeException.ThrowIfNegative(value); | ||
|
||
_positionInSuperStream = _startInSuperStream + value; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to other issue, this should call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in e525dfb. The Position setter now calls |
||
} | ||
} | ||
|
||
public override bool CanRead => _superStream.CanRead && _canRead; | ||
|
||
public override bool CanSeek => false; | ||
public override bool CanSeek => _superStream.CanSeek && !_isDisposed; | ||
rzikm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
public override bool CanWrite => false; | ||
|
||
|
@@ -366,7 +373,28 @@ async ValueTask<int> Core(Memory<byte> buffer, CancellationToken cancellationTok | |
public override long Seek(long offset, SeekOrigin origin) | ||
{ | ||
ThrowIfDisposed(); | ||
throw new NotSupportedException(SR.SeekingNotSupported); | ||
|
||
if (!CanSeek) | ||
rzikm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
throw new NotSupportedException(SR.SeekingNotSupported); | ||
} | ||
|
||
long newPositionInSuperStream = origin switch | ||
{ | ||
SeekOrigin.Begin => _startInSuperStream + offset, | ||
SeekOrigin.Current => _positionInSuperStream + offset, | ||
SeekOrigin.End => _endInSuperStream + offset, | ||
_ => throw new ArgumentOutOfRangeException(nameof(origin)), | ||
}; | ||
|
||
if (newPositionInSuperStream < _startInSuperStream) | ||
{ | ||
throw new IOException(SR.IO_SeekBeforeBegin); | ||
} | ||
|
||
_positionInSuperStream = newPositionInSuperStream; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we be calling _superStream.Seek here? That has side-effects that are observable so I would have expected that a call to Seek in the SubStream would change the position in the _superStream immediately and not be deferred to some later read. Examples of observable behavior:
I'd hope we add tests for all these cases too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in e525dfb. Both the Seek method and Position setter now call |
||
|
||
return _positionInSuperStream - _startInSuperStream; | ||
} | ||
|
||
public override void SetLength(long value) | ||
|
Uh oh!
There was an error while loading. Please reload this page.