From 08e8e19fa64e1a63657877f82f479575de1b6e67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Cant=C3=BA?= Date: Thu, 22 Dec 2022 03:27:40 -0600 Subject: [PATCH 1/3] TarReader should dispose underlying stream if leaveOpen is false (#79899) --- .../src/System/Formats/Tar/TarReader.cs | 13 +++++++++---- .../tests/TarReader/TarReader.Tests.cs | 4 ++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs b/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs index ae815dc0073b46..181b69703e25a1 100644 --- a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs +++ b/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs @@ -48,7 +48,7 @@ public TarReader(Stream archiveStream, bool leaveOpen = false) } /// - /// Disposes the current instance, and disposes the non-null instances of all the entries that were read from the archive. + /// Disposes the current instance, closes the archive stream, and disposes the non-null instances of all the entries that were read from the archive if the leaveOpen argument was set to in the constructor. /// /// The property of any entry can be replaced with a new stream. If the user decides to replace it on a instance that was obtained using a , the underlying stream gets disposed immediately, freeing the of origin from the responsibility of having to dispose it. public void Dispose() @@ -57,11 +57,16 @@ public void Dispose() { _isDisposed = true; - if (!_leaveOpen && _dataStreamsToDispose?.Count > 0) + if (!_leaveOpen) { - foreach (Stream s in _dataStreamsToDispose) + _archiveStream.Dispose(); + + if (_dataStreamsToDispose?.Count > 0) { - s.Dispose(); + foreach (Stream s in _dataStreamsToDispose) + { + s.Dispose(); + } } } } diff --git a/src/libraries/System.Formats.Tar/tests/TarReader/TarReader.Tests.cs b/src/libraries/System.Formats.Tar/tests/TarReader/TarReader.Tests.cs index 8b41a5e9bb13bd..6de99107bd7463 100644 --- a/src/libraries/System.Formats.Tar/tests/TarReader/TarReader.Tests.cs +++ b/src/libraries/System.Formats.Tar/tests/TarReader/TarReader.Tests.cs @@ -38,6 +38,8 @@ public void TarReader_LeaveOpen_False() } } + Assert.Throws(() => ms.ReadByte()); + Assert.True(dataStreams.Any()); foreach (Stream ds in dataStreams) { @@ -62,6 +64,8 @@ public void TarReader_LeaveOpen_True() } } + ms.ReadByte(); // Should not throw + Assert.True(dataStreams.Any()); foreach (Stream ds in dataStreams) { From 1fbde519deea8c61f30e552ec9684f8f96feee80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20K=C3=B6plinger?= Date: Fri, 23 Dec 2022 14:30:06 +0100 Subject: [PATCH 2/3] Dispose underlying stream in TarReader.DisposeAsync() as well (#79920) * Dispose underlying stream in TarReader.DisposeAsync() as well Same as https://github.com/dotnet/runtime/pull/79899 * Consolidate duplicated WrappedStream test helpers to Common sources * Dispose stream passed to WrappedStream --- .../tests/System/IO}/WrappedStream.cs | 4 +- .../src/System/Formats/Tar/TarReader.cs | 11 +- .../tests/System.Formats.Tar.Tests.csproj | 2 +- .../TarWriter.WriteEntry.LongFile.Tests.cs | 2 +- ...arWriter.WriteEntryAsync.LongFile.Tests.cs | 2 +- .../tests/System.IO.Compression.Tests.csproj | 2 +- .../tests/Utilities/WrappedStream.cs | 123 ------------------ 7 files changed, 13 insertions(+), 133 deletions(-) rename src/libraries/{System.Formats.Tar/tests => Common/tests/System/IO}/WrappedStream.cs (98%) delete mode 100644 src/libraries/System.IO.Compression/tests/Utilities/WrappedStream.cs diff --git a/src/libraries/System.Formats.Tar/tests/WrappedStream.cs b/src/libraries/Common/tests/System/IO/WrappedStream.cs similarity index 98% rename from src/libraries/System.Formats.Tar/tests/WrappedStream.cs rename to src/libraries/Common/tests/System/IO/WrappedStream.cs index 5697f7c09ba554..71be0b98c9a198 100644 --- a/src/libraries/System.Formats.Tar/tests/WrappedStream.cs +++ b/src/libraries/Common/tests/System/IO/WrappedStream.cs @@ -1,9 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.IO; - -namespace System.Formats.Tar +namespace System.IO { public class WrappedStream : Stream { diff --git a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs b/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs index 181b69703e25a1..541148a8582760 100644 --- a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs +++ b/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs @@ -82,11 +82,16 @@ public async ValueTask DisposeAsync() { _isDisposed = true; - if (!_leaveOpen && _dataStreamsToDispose?.Count > 0) + if (!_leaveOpen) { - foreach (Stream s in _dataStreamsToDispose) + await _archiveStream.DisposeAsync().ConfigureAwait(false); + + if (_dataStreamsToDispose?.Count > 0) { - await s.DisposeAsync().ConfigureAwait(false); + foreach (Stream s in _dataStreamsToDispose) + { + await s.DisposeAsync().ConfigureAwait(false); + } } } } diff --git a/src/libraries/System.Formats.Tar/tests/System.Formats.Tar.Tests.csproj b/src/libraries/System.Formats.Tar/tests/System.Formats.Tar.Tests.csproj index 1d30aa09d0f970..7ec67c69fc964b 100644 --- a/src/libraries/System.Formats.Tar/tests/System.Formats.Tar.Tests.csproj +++ b/src/libraries/System.Formats.Tar/tests/System.Formats.Tar.Tests.csproj @@ -68,9 +68,9 @@ - + diff --git a/src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.LongFile.Tests.cs b/src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.LongFile.Tests.cs index 6fd2166e81c35f..613e30c541d1c4 100644 --- a/src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.LongFile.Tests.cs +++ b/src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.LongFile.Tests.cs @@ -30,7 +30,7 @@ public static IEnumerable WriteEntry_LongFileSize_TheoryData() public void WriteEntry_LongFileSize(TarEntryFormat entryFormat, long size, bool unseekableStream) { // Write archive with a 8 Gb long entry. - FileStream tarFile = File.Open(GetTestFilePath(), new FileStreamOptions { Access = FileAccess.ReadWrite, Mode = FileMode.Create, Options = FileOptions.DeleteOnClose }); + using FileStream tarFile = File.Open(GetTestFilePath(), new FileStreamOptions { Access = FileAccess.ReadWrite, Mode = FileMode.Create, Options = FileOptions.DeleteOnClose }); Stream s = unseekableStream ? new WrappedStream(tarFile, tarFile.CanRead, tarFile.CanWrite, canSeek: false) : tarFile; using (TarWriter writer = new(s, leaveOpen: true)) diff --git a/src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntryAsync.LongFile.Tests.cs b/src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntryAsync.LongFile.Tests.cs index 0f6202662d6097..6d260cc9ab7bdc 100644 --- a/src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntryAsync.LongFile.Tests.cs +++ b/src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntryAsync.LongFile.Tests.cs @@ -20,7 +20,7 @@ public static IEnumerable WriteEntry_LongFileSize_TheoryDataAsync() public async Task WriteEntry_LongFileSizeAsync(TarEntryFormat entryFormat, long size, bool unseekableStream) { // Write archive with a 8 Gb long entry. - FileStream tarFile = File.Open(GetTestFilePath(), new FileStreamOptions { Access = FileAccess.ReadWrite, Mode = FileMode.Create, Options = FileOptions.DeleteOnClose }); + await using FileStream tarFile = File.Open(GetTestFilePath(), new FileStreamOptions { Access = FileAccess.ReadWrite, Mode = FileMode.Create, Options = FileOptions.DeleteOnClose }); Stream s = unseekableStream ? new WrappedStream(tarFile, tarFile.CanRead, tarFile.CanWrite, canSeek: false) : tarFile; await using (TarWriter writer = new(s, leaveOpen: true)) diff --git a/src/libraries/System.IO.Compression/tests/System.IO.Compression.Tests.csproj b/src/libraries/System.IO.Compression/tests/System.IO.Compression.Tests.csproj index ef1862eb0bf3fb..3adc39904dd46d 100644 --- a/src/libraries/System.IO.Compression/tests/System.IO.Compression.Tests.csproj +++ b/src/libraries/System.IO.Compression/tests/System.IO.Compression.Tests.csproj @@ -17,7 +17,6 @@ - @@ -36,6 +35,7 @@ + diff --git a/src/libraries/System.IO.Compression/tests/Utilities/WrappedStream.cs b/src/libraries/System.IO.Compression/tests/Utilities/WrappedStream.cs deleted file mode 100644 index c93e6662be3bee..00000000000000 --- a/src/libraries/System.IO.Compression/tests/Utilities/WrappedStream.cs +++ /dev/null @@ -1,123 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System; -using System.IO; - -internal class WrappedStream : Stream -{ - private readonly Stream _baseStream; - private readonly EventHandler _onClosed; - private bool _canRead, _canWrite, _canSeek; - - internal WrappedStream(Stream baseStream, bool canRead, bool canWrite, bool canSeek, EventHandler onClosed) - { - _baseStream = baseStream; - _onClosed = onClosed; - _canRead = canRead; - _canSeek = canSeek; - _canWrite = canWrite; - } - - internal WrappedStream(Stream baseStream, EventHandler onClosed) - : this(baseStream, true, true, true, onClosed) { } - - internal WrappedStream(Stream baseStream) : this(baseStream, null) { } - - public override void Flush() => _baseStream.Flush(); - - public override int Read(byte[] buffer, int offset, int count) - { - if (CanRead) - { - try - { - return _baseStream.Read(buffer, offset, count); - } - catch (ObjectDisposedException ex) - { - throw new NotSupportedException("This stream does not support reading", ex); - } - } - else throw new NotSupportedException("This stream does not support reading"); - } - - public override long Seek(long offset, SeekOrigin origin) - { - if (CanSeek) - { - try - { - return _baseStream.Seek(offset, origin); - } - catch (ObjectDisposedException ex) - { - throw new NotSupportedException("This stream does not support seeking", ex); - } - } - else throw new NotSupportedException("This stream does not support seeking"); - } - - public override void SetLength(long value) { _baseStream.SetLength(value); } - - public override void Write(byte[] buffer, int offset, int count) - { - if (CanWrite) - { - try - { - _baseStream.Write(buffer, offset, count); - } - catch (ObjectDisposedException ex) - { - throw new NotSupportedException("This stream does not support writing", ex); - } - } - else throw new NotSupportedException("This stream does not support writing"); - } - - public override bool CanRead => _canRead && _baseStream.CanRead; - - public override bool CanSeek => _canSeek && _baseStream.CanSeek; - - public override bool CanWrite => _canWrite && _baseStream.CanWrite; - - public override long Length => _baseStream.Length; - - public override long Position - { - get - { - if (CanSeek) - return _baseStream.Position; - throw new NotSupportedException("This stream does not support seeking"); - } - set - { - if (CanSeek) - { - try - { - _baseStream.Position = value; - } - catch (ObjectDisposedException ex) - { - throw new NotSupportedException("This stream does not support seeking", ex); - } - } - else throw new NotSupportedException("This stream does not support seeking"); - } - } - - protected override void Dispose(bool disposing) - { - if (disposing) - { - _onClosed?.Invoke(this, null); - _canRead = false; - _canWrite = false; - _canSeek = false; - } - base.Dispose(disposing); - } -} From b1f9f65772352c2727ecae3d76197813dd140657 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Cant=C3=BA?= Date: Thu, 12 Jan 2023 23:23:15 -0600 Subject: [PATCH 3/3] Dispose archive stream after the list of DataStreams (#80572) * Dispose archive stream after the list of DataStreams * Add tests for TarReader.DisposeAsync properly disposing underlying stream --- .../src/System/Formats/Tar/TarReader.cs | 8 +-- .../tests/System.Formats.Tar.Tests.csproj | 1 + .../tests/TarReader/TarReader.Async.Tests.cs | 67 +++++++++++++++++++ .../tests/TarReader/TarReader.Tests.cs | 2 +- 4 files changed, 73 insertions(+), 5 deletions(-) create mode 100644 src/libraries/System.Formats.Tar/tests/TarReader/TarReader.Async.Tests.cs diff --git a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs b/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs index 541148a8582760..d431b215d9db30 100644 --- a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs +++ b/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs @@ -59,8 +59,6 @@ public void Dispose() if (!_leaveOpen) { - _archiveStream.Dispose(); - if (_dataStreamsToDispose?.Count > 0) { foreach (Stream s in _dataStreamsToDispose) @@ -68,6 +66,8 @@ public void Dispose() s.Dispose(); } } + + _archiveStream.Dispose(); } } } @@ -84,8 +84,6 @@ public async ValueTask DisposeAsync() if (!_leaveOpen) { - await _archiveStream.DisposeAsync().ConfigureAwait(false); - if (_dataStreamsToDispose?.Count > 0) { foreach (Stream s in _dataStreamsToDispose) @@ -93,6 +91,8 @@ public async ValueTask DisposeAsync() await s.DisposeAsync().ConfigureAwait(false); } } + + await _archiveStream.DisposeAsync().ConfigureAwait(false); } } } diff --git a/src/libraries/System.Formats.Tar/tests/System.Formats.Tar.Tests.csproj b/src/libraries/System.Formats.Tar/tests/System.Formats.Tar.Tests.csproj index 7ec67c69fc964b..831da89c1ea91a 100644 --- a/src/libraries/System.Formats.Tar/tests/System.Formats.Tar.Tests.csproj +++ b/src/libraries/System.Formats.Tar/tests/System.Formats.Tar.Tests.csproj @@ -31,6 +31,7 @@ + diff --git a/src/libraries/System.Formats.Tar/tests/TarReader/TarReader.Async.Tests.cs b/src/libraries/System.Formats.Tar/tests/TarReader/TarReader.Async.Tests.cs new file mode 100644 index 00000000000000..50216ed04ccbcb --- /dev/null +++ b/src/libraries/System.Formats.Tar/tests/TarReader/TarReader.Async.Tests.cs @@ -0,0 +1,67 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Threading.Tasks; +using Xunit; + +namespace System.Formats.Tar.Tests +{ + public partial class TarReader_Tests : TarTestsBase + { + [Fact] + public async Task TarReader_LeaveOpen_False_Async() + { + await using MemoryStream ms = GetTarMemoryStream(CompressionMethod.Uncompressed, TestTarFormat.pax, "many_small_files"); + List dataStreams = new List(); + await using (TarReader reader = new TarReader(ms, leaveOpen: false)) + { + TarEntry entry; + while ((entry = await reader.GetNextEntryAsync()) != null) + { + if (entry.DataStream != null) + { + dataStreams.Add(entry.DataStream); + } + } + } + + Assert.Throws(() => ms.ReadByte()); + + Assert.True(dataStreams.Any()); + foreach (Stream ds in dataStreams) + { + Assert.Throws(() => ds.ReadByte()); + } + } + + [Fact] + public async Task TarReader_LeaveOpen_True_Async() + { + await using MemoryStream ms = GetTarMemoryStream(CompressionMethod.Uncompressed, TestTarFormat.pax, "many_small_files"); + List dataStreams = new List(); + await using (TarReader reader = new TarReader(ms, leaveOpen: true)) + { + TarEntry entry; + while ((entry = await reader.GetNextEntryAsync()) != null) + { + if (entry.DataStream != null) + { + dataStreams.Add(entry.DataStream); + } + } + } + + ms.ReadByte(); // Should not throw + + Assert.True(dataStreams.Any()); + foreach (Stream ds in dataStreams) + { + ds.ReadByte(); // Should not throw + ds.Dispose(); + } + } + } +} diff --git a/src/libraries/System.Formats.Tar/tests/TarReader/TarReader.Tests.cs b/src/libraries/System.Formats.Tar/tests/TarReader/TarReader.Tests.cs index 6de99107bd7463..479b43380939ff 100644 --- a/src/libraries/System.Formats.Tar/tests/TarReader/TarReader.Tests.cs +++ b/src/libraries/System.Formats.Tar/tests/TarReader/TarReader.Tests.cs @@ -8,7 +8,7 @@ namespace System.Formats.Tar.Tests { - public class TarReader_Tests : TarTestsBase + public partial class TarReader_Tests : TarTestsBase { [Fact] public void TarReader_NullArchiveStream() => Assert.Throws(() => new TarReader(archiveStream: null));