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

TarReader should dispose underlying stream if leaveOpen is false #79899

Merged
merged 1 commit into from
Dec 22, 2022
Merged

TarReader should dispose underlying stream if leaveOpen is false #79899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public TarReader(Stream archiveStream, bool leaveOpen = false)
}

/// <summary>
/// Disposes the current <see cref="TarReader"/> instance, and disposes the non-null <see cref="TarEntry.DataStream"/> instances of all the entries that were read from the archive.
/// Disposes the current <see cref="TarReader"/> instance, closes the archive stream, and disposes the non-null <see cref="TarEntry.DataStream"/> instances of all the entries that were read from the archive if the <c>leaveOpen</c> argument was set to <see langword="false"/> in the constructor.
/// </summary>
/// <remarks>The <see cref="TarEntry.DataStream"/> property of any entry can be replaced with a new stream. If the user decides to replace it on a <see cref="TarEntry"/> instance that was obtained using a <see cref="TarReader"/>, the underlying stream gets disposed immediately, freeing the <see cref="TarReader"/> of origin from the responsibility of having to dispose it.</remarks>
public void Dispose()
Expand All @@ -57,11 +57,16 @@ public void Dispose()
{
_isDisposed = true;

if (!_leaveOpen && _dataStreamsToDispose?.Count > 0)
if (!_leaveOpen)
{
foreach (Stream s in _dataStreamsToDispose)
_archiveStream.Dispose();
Copy link
Member

Choose a reason for hiding this comment

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

What's the relationship between the streams in _dataStreamsToDispose and _archiveStream? Do the former streams wrap the latter? I'm wondering if the archive stream should actually be disposed of after rather than before the _dataStreamsToDispose.

Copy link
Member

@carlossanlop carlossanlop Jan 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The code change is correct incorrect: The archive stream represents the whole tar archive, while the data streams represent sub-sections of the archive, each wrapping only the data section of an individual entry.

This means that all the individual data streams need to get disposed first, and the archive stream needs to be disposed at the end.

Edit: I see my mistake, I misread the diff.

Copy link
Member Author

Choose a reason for hiding this comment

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

This means that all the individual data streams need to get disposed first, and the archive stream needs to be disposed at the end.

This is the opposite of what I did here. @carlossanlop shall we fix it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.


if (_dataStreamsToDispose?.Count > 0)
{
s.Dispose();
foreach (Stream s in _dataStreamsToDispose)
{
s.Dispose();
}
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions src/libraries/System.Formats.Tar/tests/TarReader/TarReader.Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ public void TarReader_LeaveOpen_False()
}
}

Assert.Throws<ObjectDisposedException>(() => ms.ReadByte());

Assert.True(dataStreams.Any());
foreach (Stream ds in dataStreams)
{
Expand All @@ -62,6 +64,8 @@ public void TarReader_LeaveOpen_True()
}
}

ms.ReadByte(); // Should not throw

Assert.True(dataStreams.Any());
foreach (Stream ds in dataStreams)
{
Expand Down