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.

Sign up for GitHub

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

Jump to bottom

ByteRangeStream does not limit CopyToAsync(...) range as it should #206

Closed
dougbu opened this issue Nov 6, 2018 · 1 comment
Closed

ByteRangeStream does not limit CopyToAsync(...) range as it should #206

dougbu opened this issue Nov 6, 2018 · 1 comment
Assignees
@dougbu
Labels
3 - Done bug cost: XS Will take up to half a day to complete PRI: 1 - Required Must be handled in a reasonable time
Milestone
3.2.7

Comments

@dougbu
Copy link
Member

dougbu commented Nov 6, 2018

Fix is to remove the CopyToAsync(...) override in DelegatingStream. The underlying Stream's implementation of this method will do the Right Thing:tm: and use methods that are correctly overridden in DelegatingStream and its subclasses, including ByteRangeStream.

Issue originally tracked in https://devdiv.visualstudio.com/DevDiv/_workitems/edit/690342
@dougbu dougbu self-assigned this Nov 6, 2018
@dougbu dougbu added this to the 3.2.7 milestone Nov 6, 2018
@dougbu dougbu added bug PRI: 1 - Required Must be handled in a reasonable time 1 - Ready labels Nov 6, 2018
@dougbu dougbu added cost: XS Will take up to half a day to complete 2 - Working and removed 1 - Ready 2 - Working labels Nov 17, 2018
dougbu added a commit that referenced this issue Nov 25, 2018
@dougbu
Flesh out ByteRangeStream methods to make it always limit reads
2d675b0 
- #206
- `Seek(...)` was a no-op, `Position` was incorrect if `_lowerbounds != 0`, `ReadAsync(...)` read entire inner `Stream`
  - rewrite `ByteRangeStreamTest` to cover new `ByteRangeStream` members and hold fewer resources
- remove `DelegatingStream.CopyToAsync(...)` override because it copied entire inner `Stream` and was not needed
  - base implementation invokes `ReadAsync(...)`
@dougbu dougbu mentioned this issue Nov 25, 2018
Merged
dougbu added a commit that referenced this issue Nov 28, 2018
@dougbu
Flesh out ByteRangeStream methods to make it always limit reads
25f8d6e 
- #206
- `Seek(...)` was a no-op, `Position` was incorrect if `_lowerbounds != 0`, `ReadAsync(...)` read entire inner `Stream`
  - rewrite `ByteRangeStreamTest` to cover new `ByteRangeStream` members and hold fewer resources
- remove `DelegatingStream.CopyToAsync(...)` override because it copied entire inner `Stream` and was not needed
  - base implementation invokes `ReadAsync(...)`
dougbu added a commit that referenced this issue Nov 28, 2018
@dougbu
Flesh out ByteRangeStream methods to make it always limit reads (#213)
39d3064 
* Flesh out `ByteRangeStream` methods to make it always limit reads
- #206
- `Seek(...)` was a no-op, `Position` was incorrect if `_lowerbounds != 0`, `ReadAsync(...)` read entire inner `Stream`
  - rewrite `ByteRangeStreamTest` to cover new `ByteRangeStream` members and hold fewer resources
- remove `DelegatingStream.CopyToAsync(...)` override because it copied entire inner `Stream` and was not needed
  - base implementation invokes `ReadAsync(...)`
@dougbu
Copy link
Member Author

dougbu commented Nov 28, 2018

39d3064

@dougbu dougbu closed this as completed Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dougbu dougbu

Labels
3 - Done bug cost: XS Will take up to half a day to complete PRI: 1 - Required Must be handled in a reasonable time
Projects
None yet
Milestone
3.2.7
Development

No branches or pull requests
1 participant
@dougbu