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

Non-Seekable SubReadStream should not support the Length property. #63040

Closed
JimBobSquarePants opened this issue Dec 21, 2021 · 9 comments · Fixed by dotnet/dotnet-api-docs#7513
Assignees
Labels
area-System.IO.Compression documentation Documentation bug or enhancement, does not impact product or test code in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@JimBobSquarePants
Copy link

Description

According to the documentation for Stream.Length#Exceptions a NotSupportedException should be thrown when .

NotSupportedException
A class derived from Stream does not support seeking.

Reproduction Steps

The following test code should be enough to reproduce. ZipArchive sample taken from SixLabors/ImageSharp#1903

using var zipFile = new ZipArchive(new MemoryStream(
    new byte[]
    {
        0x50, 0x4B, 0x03, 0x04, 0x0A, 0x00, 0x00, 0x00, 0x00, 0x00, 0x77, 0xAF,
        0x94, 0x53, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x07, 0x00, 0x00, 0x00, 0x6D, 0x79, 0x73, 0x74, 0x65, 0x72,
        0x79, 0x50, 0x4B, 0x01, 0x02, 0x3F, 0x00, 0x0A, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x77, 0xAF, 0x94, 0x53, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x00, 0x00, 0x00, 0x07, 0x00, 0x24, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x00, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x6D,
        0x79, 0x73, 0x74, 0x65, 0x72, 0x79, 0x0A, 0x00, 0x20, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x01, 0x00, 0x18, 0x00, 0x46, 0x82, 0xFF, 0x91, 0x27, 0xF6,
        0xD7, 0x01, 0x55, 0xA1, 0xF9, 0x91, 0x27, 0xF6, 0xD7, 0x01, 0x55, 0xA1,
        0xF9, 0x91, 0x27, 0xF6, 0xD7, 0x01, 0x50, 0x4B, 0x05, 0x06, 0x00, 0x00,
        0x00, 0x00, 0x01, 0x00, 0x01, 0x00, 0x59, 0x00, 0x00, 0x00, 0x25, 0x00,
        0x00, 0x00, 0x00, 0x00
    }));
using Stream stream = zipFile.Entries[0].Open();
Assert.Throws<NotSupportedException>(() => stream.Length);

Expected behavior

The stream should throw a NotSupportedException

Actual behavior

The stream returns the length.

Regression?

No response

Known Workarounds

No response

Configuration

.NET 6 Any OS and architecture.

Other information

No response

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Dec 21, 2021
@ghost
Copy link

ghost commented Dec 21, 2021

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

According to the documentation for Stream.Length#Exceptions a NotSupportedException should be thrown when .

NotSupportedException
A class derived from Stream does not support seeking.

Reproduction Steps

The following test code should be enough to reproduce. ZipArchive sample taken from SixLabors/ImageSharp#1903

using var zipFile = new ZipArchive(new MemoryStream(
    new byte[]
    {
        0x50, 0x4B, 0x03, 0x04, 0x0A, 0x00, 0x00, 0x00, 0x00, 0x00, 0x77, 0xAF,
        0x94, 0x53, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x07, 0x00, 0x00, 0x00, 0x6D, 0x79, 0x73, 0x74, 0x65, 0x72,
        0x79, 0x50, 0x4B, 0x01, 0x02, 0x3F, 0x00, 0x0A, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x77, 0xAF, 0x94, 0x53, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x00, 0x00, 0x00, 0x07, 0x00, 0x24, 0x00, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x00, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x6D,
        0x79, 0x73, 0x74, 0x65, 0x72, 0x79, 0x0A, 0x00, 0x20, 0x00, 0x00, 0x00,
        0x00, 0x00, 0x01, 0x00, 0x18, 0x00, 0x46, 0x82, 0xFF, 0x91, 0x27, 0xF6,
        0xD7, 0x01, 0x55, 0xA1, 0xF9, 0x91, 0x27, 0xF6, 0xD7, 0x01, 0x55, 0xA1,
        0xF9, 0x91, 0x27, 0xF6, 0xD7, 0x01, 0x50, 0x4B, 0x05, 0x06, 0x00, 0x00,
        0x00, 0x00, 0x01, 0x00, 0x01, 0x00, 0x59, 0x00, 0x00, 0x00, 0x25, 0x00,
        0x00, 0x00, 0x00, 0x00
    }));
using Stream stream = zipFile.Entries[0].Open();
Assert.Throws<NotSupportedException>(() => stream.Length);

Expected behavior

The stream should throw a NotSupportedException

Actual behavior

The stream returns the length.

Regression?

No response

Known Workarounds

No response

Configuration

.NET 6 Any OS and architecture.

Other information

No response

Author: JimBobSquarePants
Assignees: -
Labels:

area-System.IO.Compression, untriaged

Milestone: -

@mgravell
Copy link
Member

mgravell commented Dec 21, 2021

Counter opinion (entirely subjective): if the length is knowable, it is IMO fine and desirable to report it. For example, a bounded once-only stream may be known-length yet non-seekable, and in that case: it remains desirable to report the length, for use in things like headers / prefixes, to avoid an extra buffer/count step. If the consuming code cares about seekability, it should test CanSeek.

In other words, I'm saying that personally I'd interpret this as a "may throw", not a "must throw".

@JimBobSquarePants
Copy link
Author

The CanSeek docs agree with you @mgravell (Thanks @scalablecory).

I'm fine with closing this then and raising something against the docs repo instead. Length reads (to me anyway) as explicit.

@adamsitnik
Copy link
Member

@JimBobSquarePants would it be enough if we have updated the docs in the following way?

- A class derived from Stream does not support seeking.
+ A class derived from Stream does not support seeking and the length is unknown.

@adamsitnik adamsitnik added documentation Documentation bug or enhancement, does not impact product or test code and removed untriaged New issue has not been triaged by the area owner labels Dec 21, 2021
@JimBobSquarePants
Copy link
Author

@adamsitnik Yeah that's much better! 👍

@Sergio0694
Copy link
Contributor

Possibly related to #43448 😊

@antonfirsov
Copy link
Member

antonfirsov commented Dec 21, 2021

Somewhat related:

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 = stream.Length * 2;
Assert.Equal(stream.Length * 2, stream.Position);

Or going more extreme:

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

@carlossanlop carlossanlop added the in-pr There is an active PR which will close this issue when it is merged label Dec 22, 2021
@zmj
Copy link

zmj commented Dec 22, 2021

For example, a bounded once-only stream may be known-length yet non-seekable, and in that case: it remains desirable to report the length, for use in things like headers / prefixes, to avoid an extra buffer/count step. If the consuming code cares about seekability, it should test CanSeek.

On the consuming side of Streams like this, it would be nice to have a non-throwing, non-CanSeek way to try to get a length.

@jeffhandley jeffhandley added this to the 7.0.0 milestone Jul 10, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO.Compression documentation Documentation bug or enhancement, does not impact product or test code in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants