Skip to content

Conversation

@JimBobSquarePants
Copy link
Member

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

According to the docs for Stream.Position

Seeking to any location beyond the length of the stream is supported.

Removing our explicit throw make the implementation compliant. Fix #1903

@codecov
Copy link

codecov bot commented Dec 21, 2021

Codecov Report

Merging #1907 (7ed9bb6) into master (3c421bb) will increase coverage by 0%.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1907   +/-   ##
======================================
  Coverage      87%     87%           
======================================
  Files         962     962           
  Lines       51121   51120    -1     
  Branches     6342    6342           
======================================
+ Hits        44925   44928    +3     
+ Misses       5148    5145    -3     
+ Partials     1048    1047    -1     
Flag Coverage Δ
unittests 87% <ø> (+<1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/ImageSharp/IO/ChunkedMemoryStream.cs 83% <ø> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e20410c...7ed9bb6. Read the comment docs.

@antonfirsov
Copy link
Member

antonfirsov commented Dec 21, 2021

Seeking to any location beyond the length of the stream is supported.

I wonder what are detailed semantics for this. For example is it a requirement for Stream.Position to roundtrip?

stream.Position = long.MaxValue;
Assert.Equal(long.MaxValue, stream.Position);

@JimBobSquarePants
Copy link
Member Author

stream.Position = long.MaxValue;
Assert.Equal(long.MaxValue, stream.Position);

That actually throws an ArgumentOutOfRangeException with MemoryStream. int.MaxValue is the largest it allows. FileStream however will accept it.

I based (copied mostly) the implementation from an old stream in the System.Runtime.Remoting.Channels namespace where they explicitly threw. I'm not that concerned about consistency since it has a very constrained and internal use case.

@ynse01 ynse01 mentioned this pull request Jan 2, 2022
4 tasks
@JimBobSquarePants JimBobSquarePants requested a review from a team January 4, 2022 12:25
Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

LGTM, since we are good enough. Still would be happy if someone clarified (in dotnet/runtime#63040 or somewhere else) what are conformance expectations around the "seeking beyond the length" behavior, but not going to push further to chase the answer.

@JimBobSquarePants JimBobSquarePants merged commit a5ae98c into master Jan 4, 2022
@JimBobSquarePants JimBobSquarePants deleted the js/chunkstream-position branch January 4, 2022 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Image.Identify throws ArgumentOutOfRangeException on empty unseekable stream

3 participants