From 6ae715b153ee21d27aa0caa0d42ed8ff4a86d61c Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Tue, 10 Jul 2018 10:15:21 -0700 Subject: [PATCH 01/23] Move the TestStream and ForwardOnlyStream to Mocks folder Move the `TestStream` and `ForwardOnlyStream` to Mocks/ to separate them from the test classes. --- tests/SharpCompress.Test/{ => Mocks}/ForwardOnlyStream.cs | 2 +- tests/SharpCompress.Test/{ => Mocks}/TestStream.cs | 2 +- tests/SharpCompress.Test/ReaderTests.cs | 1 + tests/SharpCompress.Test/Tar/TarReaderTests.cs | 1 + tests/SharpCompress.Test/Zip/ZipReaderTests.cs | 1 + 5 files changed, 5 insertions(+), 2 deletions(-) rename tests/SharpCompress.Test/{ => Mocks}/ForwardOnlyStream.cs (97%) rename tests/SharpCompress.Test/{ => Mocks}/TestStream.cs (97%) diff --git a/tests/SharpCompress.Test/ForwardOnlyStream.cs b/tests/SharpCompress.Test/Mocks/ForwardOnlyStream.cs similarity index 97% rename from tests/SharpCompress.Test/ForwardOnlyStream.cs rename to tests/SharpCompress.Test/Mocks/ForwardOnlyStream.cs index c20b9b99..482706a3 100644 --- a/tests/SharpCompress.Test/ForwardOnlyStream.cs +++ b/tests/SharpCompress.Test/Mocks/ForwardOnlyStream.cs @@ -1,7 +1,7 @@ using System; using System.IO; -namespace SharpCompress.Test +namespace SharpCompress.Test.Mocks { public class ForwardOnlyStream : Stream { diff --git a/tests/SharpCompress.Test/TestStream.cs b/tests/SharpCompress.Test/Mocks/TestStream.cs similarity index 97% rename from tests/SharpCompress.Test/TestStream.cs rename to tests/SharpCompress.Test/Mocks/TestStream.cs index f9e0b713..d3f741c6 100644 --- a/tests/SharpCompress.Test/TestStream.cs +++ b/tests/SharpCompress.Test/Mocks/TestStream.cs @@ -1,6 +1,6 @@ using System.IO; -namespace SharpCompress.Test +namespace SharpCompress.Test.Mocks { public class TestStream : Stream { diff --git a/tests/SharpCompress.Test/ReaderTests.cs b/tests/SharpCompress.Test/ReaderTests.cs index aa2ec9ce..5b052bec 100644 --- a/tests/SharpCompress.Test/ReaderTests.cs +++ b/tests/SharpCompress.Test/ReaderTests.cs @@ -3,6 +3,7 @@ using System.IO; using SharpCompress.Common; using SharpCompress.IO; using SharpCompress.Readers; +using SharpCompress.Test.Mocks; using Xunit; namespace SharpCompress.Test diff --git a/tests/SharpCompress.Test/Tar/TarReaderTests.cs b/tests/SharpCompress.Test/Tar/TarReaderTests.cs index 602467a3..acd73eec 100644 --- a/tests/SharpCompress.Test/Tar/TarReaderTests.cs +++ b/tests/SharpCompress.Test/Tar/TarReaderTests.cs @@ -3,6 +3,7 @@ using System.IO; using SharpCompress.Common; using SharpCompress.Readers; using SharpCompress.Readers.Tar; +using SharpCompress.Test.Mocks; using Xunit; namespace SharpCompress.Test.Tar diff --git a/tests/SharpCompress.Test/Zip/ZipReaderTests.cs b/tests/SharpCompress.Test/Zip/ZipReaderTests.cs index aa7c6aeb..2da74330 100644 --- a/tests/SharpCompress.Test/Zip/ZipReaderTests.cs +++ b/tests/SharpCompress.Test/Zip/ZipReaderTests.cs @@ -4,6 +4,7 @@ using SharpCompress.Common; using SharpCompress.IO; using SharpCompress.Readers; using SharpCompress.Readers.Zip; +using SharpCompress.Test.Mocks; using SharpCompress.Writers; using Xunit; From 084c5e268620b110ab0e48cd9204fd5fe32ed746 Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Tue, 10 Jul 2018 10:20:52 -0700 Subject: [PATCH 02/23] Rename StreamTests.cs --> LzmaStreamTests.cs --- .../Streams/{StreamTests.cs => LzmaStreamTests.cs} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename tests/SharpCompress.Test/Streams/{StreamTests.cs => LzmaStreamTests.cs} (94%) diff --git a/tests/SharpCompress.Test/Streams/StreamTests.cs b/tests/SharpCompress.Test/Streams/LzmaStreamTests.cs similarity index 94% rename from tests/SharpCompress.Test/Streams/StreamTests.cs rename to tests/SharpCompress.Test/Streams/LzmaStreamTests.cs index daa3711c..3e41ee83 100644 --- a/tests/SharpCompress.Test/Streams/StreamTests.cs +++ b/tests/SharpCompress.Test/Streams/LzmaStreamTests.cs @@ -4,7 +4,7 @@ using Xunit; namespace SharpCompress.Test.Streams { - public class StreamTests + public class LzmaStreamTests { [Fact] public void TestLzma2Decompress1Byte() From a35c66e166b67bc2677604c3bea05d7f244ef5af Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Tue, 10 Jul 2018 10:22:36 -0700 Subject: [PATCH 03/23] Move RewindableStreamTest.cs to the Streams/ folder --- tests/SharpCompress.Test/{ => Streams}/RewindableStreamTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename tests/SharpCompress.Test/{ => Streams}/RewindableStreamTest.cs (98%) diff --git a/tests/SharpCompress.Test/RewindableStreamTest.cs b/tests/SharpCompress.Test/Streams/RewindableStreamTest.cs similarity index 98% rename from tests/SharpCompress.Test/RewindableStreamTest.cs rename to tests/SharpCompress.Test/Streams/RewindableStreamTest.cs index 52c58227..14beab4f 100644 --- a/tests/SharpCompress.Test/RewindableStreamTest.cs +++ b/tests/SharpCompress.Test/Streams/RewindableStreamTest.cs @@ -2,7 +2,7 @@ using SharpCompress.IO; using Xunit; -namespace SharpCompress.Test +namespace SharpCompress.Test.Streams { public class RewindableStreamTest { From c2bf540057e8711b21aac7e6693eded14c8af8e4 Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 12:12:30 -0700 Subject: [PATCH 04/23] Close verification streams in TestBase.CompareArchivesByPath --- tests/SharpCompress.Test/TestBase.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/SharpCompress.Test/TestBase.cs b/tests/SharpCompress.Test/TestBase.cs index f9438673..da73fab6 100644 --- a/tests/SharpCompress.Test/TestBase.cs +++ b/tests/SharpCompress.Test/TestBase.cs @@ -205,8 +205,7 @@ namespace SharpCompress.Test } protected void CompareArchivesByPath(string file1, string file2) { - ReaderOptions readerOptions = new ReaderOptions(); - + ReaderOptions readerOptions = new ReaderOptions { LeaveStreamOpen = false }; readerOptions.ArchiveEncoding.Default = Encoding.GetEncoding(866); //don't compare the order. OS X reads files from the file system in a different order therefore makes the archive ordering different From 6c2e5e116436fef65648b9dbd378060ccbdbf335 Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 12:19:34 -0700 Subject: [PATCH 05/23] Cleanup NonDisposingStream for reuse - Remove the duplicate `GC.SuppressFinalization` call (called in `System.IO.Stream) - Improve the `ThrowOnDispose` error message --- src/SharpCompress/IO/NonDisposingStream.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/SharpCompress/IO/NonDisposingStream.cs b/src/SharpCompress/IO/NonDisposingStream.cs index 2c9e72c4..d78b1bfb 100644 --- a/src/SharpCompress/IO/NonDisposingStream.cs +++ b/src/SharpCompress/IO/NonDisposingStream.cs @@ -15,10 +15,9 @@ namespace SharpCompress.IO protected override void Dispose(bool disposing) { - GC.SuppressFinalize(this); if (ThrowOnDispose) { - throw new InvalidOperationException(); + throw new InvalidOperationException($"Attempt to dispose of a {nameof(NonDisposingStream)} when {nameof(ThrowOnDispose)} is {ThrowOnDispose}"); } } From cab1ce3d0cda16f931a2348f58873338646def5d Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 12:54:40 -0700 Subject: [PATCH 06/23] Update sub-streams to uniformly inherit from NonDisposingStream Update the sub-stream classes to all inherit from `NonDisposingStream`. This allows them to correctly implement the `Dispose` pattern, and delegate the actual disposal to `NonDisposingStream`. In doing so, we need to remove some redundant overrides from `NonDisposingStream`, otherwise `BufferedSubStream` would use the overrides inherited from `NonDisposingStream` instead of the ones inherited from `Stream` (i.e. delegate `ReadByte` to `Read`). --- .../Common/Tar/TarReadOnlySubStream.cs | 11 +++++------ src/SharpCompress/IO/BufferedSubStream.cs | 15 ++------------- src/SharpCompress/IO/CountingWritableSubStream.cs | 13 +++++-------- src/SharpCompress/IO/NonDisposingStream.cs | 10 ---------- src/SharpCompress/IO/ReadOnlySubStream.cs | 2 +- 5 files changed, 13 insertions(+), 38 deletions(-) diff --git a/src/SharpCompress/Common/Tar/TarReadOnlySubStream.cs b/src/SharpCompress/Common/Tar/TarReadOnlySubStream.cs index 25300840..ed808d42 100644 --- a/src/SharpCompress/Common/Tar/TarReadOnlySubStream.cs +++ b/src/SharpCompress/Common/Tar/TarReadOnlySubStream.cs @@ -1,16 +1,16 @@ -using System; +using SharpCompress.IO; +using System; using System.IO; namespace SharpCompress.Common.Tar { - internal class TarReadOnlySubStream : Stream + internal class TarReadOnlySubStream : NonDisposingStream { private bool _isDisposed; private long _amountRead; - public TarReadOnlySubStream(Stream stream, long bytesToRead) + public TarReadOnlySubStream(Stream stream, long bytesToRead) : base(stream, throwOnDispose: false) { - Stream = stream; BytesLeftToRead = bytesToRead; } @@ -36,12 +36,11 @@ namespace SharpCompress.Common.Tar var buffer = new byte[skipBytes]; Stream.ReadFully(buffer); } + base.Dispose(disposing); } private long BytesLeftToRead { get; set; } - public Stream Stream { get; } - public override bool CanRead => true; public override bool CanSeek => false; diff --git a/src/SharpCompress/IO/BufferedSubStream.cs b/src/SharpCompress/IO/BufferedSubStream.cs index f08be705..9b6ae6cd 100644 --- a/src/SharpCompress/IO/BufferedSubStream.cs +++ b/src/SharpCompress/IO/BufferedSubStream.cs @@ -3,33 +3,22 @@ using System.IO; namespace SharpCompress.IO { - internal class BufferedSubStream : Stream + internal class BufferedSubStream : NonDisposingStream { private long position; private int cacheOffset; private int cacheLength; private readonly byte[] cache; - public BufferedSubStream(Stream stream, long origin, long bytesToRead) + public BufferedSubStream(Stream stream, long origin, long bytesToRead) : base(stream, throwOnDispose: false) { - Stream = stream; position = origin; BytesLeftToRead = bytesToRead; cache = new byte[32 << 10]; } - protected override void Dispose(bool disposing) - { - if (disposing) - { - //Stream.Dispose(); - } - } - private long BytesLeftToRead { get; set; } - public Stream Stream { get; } - public override bool CanRead => true; public override bool CanSeek => false; diff --git a/src/SharpCompress/IO/CountingWritableSubStream.cs b/src/SharpCompress/IO/CountingWritableSubStream.cs index 17ddf6bb..b94303bf 100644 --- a/src/SharpCompress/IO/CountingWritableSubStream.cs +++ b/src/SharpCompress/IO/CountingWritableSubStream.cs @@ -3,13 +3,10 @@ using System.IO; namespace SharpCompress.IO { - internal class CountingWritableSubStream : Stream + internal class CountingWritableSubStream : NonDisposingStream { - private readonly Stream writableStream; - - internal CountingWritableSubStream(Stream stream) + internal CountingWritableSubStream(Stream stream) : base(stream, throwOnDispose: false) { - writableStream = stream; } public ulong Count { get; private set; } @@ -22,7 +19,7 @@ namespace SharpCompress.IO public override void Flush() { - writableStream.Flush(); + Stream.Flush(); } public override long Length => throw new NotSupportedException(); @@ -46,13 +43,13 @@ namespace SharpCompress.IO public override void Write(byte[] buffer, int offset, int count) { - writableStream.Write(buffer, offset, count); + Stream.Write(buffer, offset, count); Count += (uint)count; } public override void WriteByte(byte value) { - writableStream.WriteByte(value); + Stream.WriteByte(value); ++Count; } } diff --git a/src/SharpCompress/IO/NonDisposingStream.cs b/src/SharpCompress/IO/NonDisposingStream.cs index d78b1bfb..9c7c4059 100644 --- a/src/SharpCompress/IO/NonDisposingStream.cs +++ b/src/SharpCompress/IO/NonDisposingStream.cs @@ -43,11 +43,6 @@ namespace SharpCompress.IO return Stream.Read(buffer, offset, count); } - public override int ReadByte() - { - return Stream.ReadByte(); - } - public override long Seek(long offset, SeekOrigin origin) { return Stream.Seek(offset, origin); @@ -62,10 +57,5 @@ namespace SharpCompress.IO { Stream.Write(buffer, offset, count); } - - public override void WriteByte(byte value) - { - Stream.WriteByte(value); - } } } \ No newline at end of file diff --git a/src/SharpCompress/IO/ReadOnlySubStream.cs b/src/SharpCompress/IO/ReadOnlySubStream.cs index 8747019a..e05921ca 100644 --- a/src/SharpCompress/IO/ReadOnlySubStream.cs +++ b/src/SharpCompress/IO/ReadOnlySubStream.cs @@ -11,7 +11,7 @@ namespace SharpCompress.IO } public ReadOnlySubStream(Stream stream, long? origin, long bytesToRead) - : base(stream, false) + : base(stream, throwOnDispose: false) { if (origin != null) { From 16524717ba346407c87d5d9a3b23cae2ba6380f7 Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 12:55:11 -0700 Subject: [PATCH 07/23] Fix Stream leak in ArchiveFactory `ArchiveFactory.Open` has two overloads that take `string` or `FileInfo` (string delegates to FileInfo). Both of these implementations open a `Stream` with the default `ReaderOptions`, which leaves the stream open, resulting in a leak. The fix is to set `LeaveOpen` to `false` if no options were provided. Note that if a user was provding options and `LeaveOpen` was set to `true`, the code did and will still leak. --- src/SharpCompress/Archives/ArchiveFactory.cs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/SharpCompress/Archives/ArchiveFactory.cs b/src/SharpCompress/Archives/ArchiveFactory.cs index 89cffcc1..41435e22 100644 --- a/src/SharpCompress/Archives/ArchiveFactory.cs +++ b/src/SharpCompress/Archives/ArchiveFactory.cs @@ -92,7 +92,7 @@ namespace SharpCompress.Archives public static IArchive Open(string filePath, ReaderOptions options = null) { filePath.CheckNotNullOrEmpty("filePath"); - return Open(new FileInfo(filePath), options ?? new ReaderOptions()); + return Open(new FileInfo(filePath), options); } /// @@ -103,36 +103,31 @@ namespace SharpCompress.Archives public static IArchive Open(FileInfo fileInfo, ReaderOptions options = null) { fileInfo.CheckNotNull("fileInfo"); - options = options ?? new ReaderOptions(); + options = options ?? new ReaderOptions { LeaveStreamOpen = false }; using (var stream = fileInfo.OpenRead()) { if (ZipArchive.IsZipFile(stream, null)) { - stream.Dispose(); return ZipArchive.Open(fileInfo, options); } stream.Seek(0, SeekOrigin.Begin); if (SevenZipArchive.IsSevenZipFile(stream)) { - stream.Dispose(); return SevenZipArchive.Open(fileInfo, options); } stream.Seek(0, SeekOrigin.Begin); if (GZipArchive.IsGZipFile(stream)) { - stream.Dispose(); return GZipArchive.Open(fileInfo, options); } stream.Seek(0, SeekOrigin.Begin); if (RarArchive.IsRarFile(stream, options)) { - stream.Dispose(); return RarArchive.Open(fileInfo, options); } stream.Seek(0, SeekOrigin.Begin); if (TarArchive.IsTarFile(stream)) { - stream.Dispose(); return TarArchive.Open(fileInfo, options); } throw new InvalidOperationException("Cannot determine compressed stream type. Supported Archive Formats: Zip, GZip, Tar, Rar, 7Zip"); From 4cd80e96f3c514383e7921741061f4716cd4b9e4 Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 14:14:16 -0700 Subject: [PATCH 08/23] Simplify GZip bad compression test --- tests/SharpCompress.Test/GZip/GZipWriterTests.cs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/tests/SharpCompress.Test/GZip/GZipWriterTests.cs b/tests/SharpCompress.Test/GZip/GZipWriterTests.cs index 3d0e6e20..f9edbb52 100644 --- a/tests/SharpCompress.Test/GZip/GZipWriterTests.cs +++ b/tests/SharpCompress.Test/GZip/GZipWriterTests.cs @@ -42,16 +42,12 @@ namespace SharpCompress.Test.GZip public void GZip_Writer_Generic_Bad_Compression() { Assert.Throws(() => - { - using (Stream stream = File.OpenWrite(Path.Combine(SCRATCH_FILES_PATH, "Tar.tar.gz"))) - using (var writer = WriterFactory.Open(stream, ArchiveType.GZip, CompressionType.BZip2)) - { - writer.Write("Tar.tar", Path.Combine(TEST_ARCHIVES_PATH, "Tar.tar")); - } - CompareArchivesByPath(Path.Combine(SCRATCH_FILES_PATH, "Tar.tar.gz"), - Path.Combine(TEST_ARCHIVES_PATH, "Tar.tar.gz")); - - }); + { + using (Stream stream = File.OpenWrite(Path.Combine(SCRATCH_FILES_PATH, "Tar.tar.gz"))) + using (var writer = WriterFactory.Open(stream, ArchiveType.GZip, CompressionType.BZip2)) + { + } + }); } } } From c6cf0d40ee6868d5ee04e9b5dff63065acc194f6 Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 14:15:05 -0700 Subject: [PATCH 09/23] Simplify ReaderTests The `IEnumerable` version of `ReaderTests` is unused, so delete it to simplify the code. --- tests/SharpCompress.Test/ReaderTests.cs | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/tests/SharpCompress.Test/ReaderTests.cs b/tests/SharpCompress.Test/ReaderTests.cs index 5b052bec..1fb6d865 100644 --- a/tests/SharpCompress.Test/ReaderTests.cs +++ b/tests/SharpCompress.Test/ReaderTests.cs @@ -1,5 +1,4 @@ -using System.Collections.Generic; -using System.IO; +using System.IO; using SharpCompress.Common; using SharpCompress.IO; using SharpCompress.Readers; @@ -13,22 +12,12 @@ namespace SharpCompress.Test protected void Read(string testArchive, CompressionType expectedCompression) { testArchive = Path.Combine(TEST_ARCHIVES_PATH, testArchive); - Read(testArchive.AsEnumerable(), expectedCompression); - } - protected void Read(IEnumerable testArchives, CompressionType expectedCompression) - { - foreach (var path in testArchives) + using (var stream = new NonDisposingStream(new ForwardOnlyStream(File.OpenRead(testArchive)), true)) + using (var reader = ReaderFactory.Open(stream, new ReaderOptions { LeaveStreamOpen = true })) { - using (var stream = new NonDisposingStream(new ForwardOnlyStream(File.OpenRead(path)), true)) - using (var reader = ReaderFactory.Open(stream, new ReaderOptions() - { - LeaveStreamOpen = true - })) - { - UseReader(this, reader, expectedCompression); - stream.ThrowOnDispose = false; - } + UseReader(this, reader, expectedCompression); + stream.ThrowOnDispose = false; } } From 0473ec1626a729fe851df5bf09db0ab30b78fb63 Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 14:18:24 -0700 Subject: [PATCH 10/23] Open test archives as read Update `RarHeaderFactoryTests` and `GZipArchiveTests` to open the test readers as `FileAccess.Read` and `FileShare.Read` to prevent issues with multiple test from trying to open exclusive access to files. --- tests/SharpCompress.Test/GZip/GZipArchiveTests.cs | 8 ++++---- tests/SharpCompress.Test/Rar/RarHeaderFactoryTest.cs | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/SharpCompress.Test/GZip/GZipArchiveTests.cs b/tests/SharpCompress.Test/GZip/GZipArchiveTests.cs index 120b2d19..8476724a 100644 --- a/tests/SharpCompress.Test/GZip/GZipArchiveTests.cs +++ b/tests/SharpCompress.Test/GZip/GZipArchiveTests.cs @@ -17,7 +17,7 @@ namespace SharpCompress.Test.GZip [Fact] public void GZip_Archive_Generic() { - using (Stream stream = File.Open(Path.Combine(TEST_ARCHIVES_PATH, "Tar.tar.gz"), FileMode.Open)) + using (Stream stream = File.OpenRead(Path.Combine(TEST_ARCHIVES_PATH, "Tar.tar.gz"))) using (var archive = ArchiveFactory.Open(stream)) { var entry = archive.Entries.First(); @@ -30,7 +30,7 @@ namespace SharpCompress.Test.GZip [Fact] public void GZip_Archive() { - using (Stream stream = File.Open(Path.Combine(TEST_ARCHIVES_PATH, "Tar.tar.gz"), FileMode.Open)) + using (Stream stream = File.OpenRead(Path.Combine(TEST_ARCHIVES_PATH, "Tar.tar.gz"))) using (var archive = GZipArchive.Open(stream)) { var entry = archive.Entries.First(); @@ -45,7 +45,7 @@ namespace SharpCompress.Test.GZip public void GZip_Archive_NoAdd() { string jpg = Path.Combine(ORIGINAL_FILES_PATH, "jpg", "test.jpg"); - using (Stream stream = File.Open(Path.Combine(TEST_ARCHIVES_PATH, "Tar.tar.gz"), FileMode.Open)) + using (Stream stream = File.OpenRead(Path.Combine(TEST_ARCHIVES_PATH, "Tar.tar.gz"))) using (var archive = GZipArchive.Open(stream)) { Assert.Throws(() => archive.AddEntry("jpg\\test.jpg", jpg)); @@ -58,7 +58,7 @@ namespace SharpCompress.Test.GZip public void GZip_Archive_Multiple_Reads() { var inputStream = new MemoryStream(); - using (var fileStream = File.Open(Path.Combine(TEST_ARCHIVES_PATH, "Tar.tar.gz"), FileMode.Open)) + using (var fileStream = File.OpenRead(Path.Combine(TEST_ARCHIVES_PATH, "Tar.tar.gz"))) { fileStream.CopyTo(inputStream); inputStream.Position = 0; diff --git a/tests/SharpCompress.Test/Rar/RarHeaderFactoryTest.cs b/tests/SharpCompress.Test/Rar/RarHeaderFactoryTest.cs index 79784b55..283c7607 100644 --- a/tests/SharpCompress.Test/Rar/RarHeaderFactoryTest.cs +++ b/tests/SharpCompress.Test/Rar/RarHeaderFactoryTest.cs @@ -61,7 +61,7 @@ namespace SharpCompress.Test.Rar private FileStream GetReaderStream(string testArchive) { - return new FileStream(Path.Combine(TEST_ARCHIVES_PATH, testArchive), FileMode.Open); + return new FileStream(Path.Combine(TEST_ARCHIVES_PATH, testArchive), FileMode.Open, FileAccess.Read); } } } From ee4ae661d78c38fd7f37103e369ecc0abbe5e789 Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 14:48:10 -0700 Subject: [PATCH 11/23] Refactor ListeningStream Refactor `ListeningStream`: - Override of `WriteByte` was redundant and removed - Make `Dispose` delegate to base class --- src/SharpCompress/IO/ListeningStream.cs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/SharpCompress/IO/ListeningStream.cs b/src/SharpCompress/IO/ListeningStream.cs index a5a76dd1..1bab99d2 100644 --- a/src/SharpCompress/IO/ListeningStream.cs +++ b/src/SharpCompress/IO/ListeningStream.cs @@ -20,6 +20,7 @@ namespace SharpCompress.IO { Stream.Dispose(); } + base.Dispose(disposing); } public Stream Stream { get; } @@ -74,10 +75,5 @@ namespace SharpCompress.IO { Stream.Write(buffer, offset, count); } - - public override void WriteByte(byte value) - { - Stream.WriteByte(value); - } } } \ No newline at end of file From 6d1d62fd32eb10c6b5b353814572e7db11e275a4 Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 14:48:46 -0700 Subject: [PATCH 12/23] Delete AppendingStream `AppendingStream` is unused, so rather than refactor it, just delete it. --- src/SharpCompress/IO/AppendingStream.cs | 74 ------------------------- 1 file changed, 74 deletions(-) delete mode 100644 src/SharpCompress/IO/AppendingStream.cs diff --git a/src/SharpCompress/IO/AppendingStream.cs b/src/SharpCompress/IO/AppendingStream.cs deleted file mode 100644 index 4a024fb1..00000000 --- a/src/SharpCompress/IO/AppendingStream.cs +++ /dev/null @@ -1,74 +0,0 @@ -using System; -using System.Collections.Generic; -using System.IO; - -namespace SharpCompress.IO -{ - public class ReadOnlyAppendingStream : Stream - { - private readonly Queue streams; - private Stream current; - - public ReadOnlyAppendingStream(IEnumerable streams) - { - this.streams = new Queue(streams); - } - - public override bool CanRead => true; - - public override bool CanSeek => false; - - public override bool CanWrite => false; - - public override void Flush() - { - throw new NotImplementedException(); - } - - public override long Length => throw new NotImplementedException(); - - public override long Position { get => throw new NotImplementedException(); set => throw new NotImplementedException(); } - - public override int Read(byte[] buffer, int offset, int count) - { - if (current == null && streams.Count == 0) - { - return -1; - } - if (current == null) - { - current = streams.Dequeue(); - } - int totalRead = 0; - while (totalRead < count) - { - int read = current.Read(buffer, offset + totalRead, count - totalRead); - if (read <= 0) - { - if (streams.Count == 0) - { - return totalRead; - } - current = streams.Dequeue(); - } - totalRead += read; - } - return totalRead; - } - - public override long Seek(long offset, SeekOrigin origin) - { - throw new NotImplementedException(); - } - - public override void SetLength(long value) - { - throw new NotImplementedException(); - } - - public override void Write(byte[] buffer, int offset, int count) - { - throw new NotImplementedException(); - } - } -} \ No newline at end of file From 6c25322465c59b55136faad17bf0e196ccd1618b Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 14:49:42 -0700 Subject: [PATCH 13/23] Follow best-practices for Dispose in Volume and ForwardOnlyStream Update `Volume` and `ForwardOnlyStream` to follow the project's general pattern and best-practices for `Dispose` --- src/SharpCompress/Common/Volume.cs | 18 +++++++++++------- .../Mocks/ForwardOnlyStream.cs | 12 +++++++++--- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/SharpCompress/Common/Volume.cs b/src/SharpCompress/Common/Volume.cs index 86d968c0..b937b281 100644 --- a/src/SharpCompress/Common/Volume.cs +++ b/src/SharpCompress/Common/Volume.cs @@ -1,4 +1,5 @@ -using System.IO; +using System; +using System.IO; using SharpCompress.IO; using SharpCompress.Readers; @@ -33,15 +34,18 @@ namespace SharpCompress.Common /// public virtual bool IsMultiVolume => true; - private bool _disposed; + protected virtual void Dispose(bool disposing) + { + if (disposing) + { + _actualStream.Dispose(); + } + } public void Dispose() { - if (!_disposed) - { - _actualStream.Dispose(); - _disposed = true; - } + Dispose(true); + GC.SuppressFinalize(this); } } } \ No newline at end of file diff --git a/tests/SharpCompress.Test/Mocks/ForwardOnlyStream.cs b/tests/SharpCompress.Test/Mocks/ForwardOnlyStream.cs index 482706a3..b5e5bbf1 100644 --- a/tests/SharpCompress.Test/Mocks/ForwardOnlyStream.cs +++ b/tests/SharpCompress.Test/Mocks/ForwardOnlyStream.cs @@ -16,9 +16,15 @@ namespace SharpCompress.Test.Mocks protected override void Dispose(bool disposing) { - base.Dispose(disposing); - stream.Dispose(); - IsDisposed = true; + if (!IsDisposed) + { + if (disposing) + { + stream.Dispose(); + IsDisposed = true; + base.Dispose(disposing); + } + } } public override bool CanRead => true; From 98558c5ba9677442887090620cfcc2e966ac1b54 Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 14:55:20 -0700 Subject: [PATCH 14/23] Refactor TestStream constructor Refactor the `TestStream` constructor so by default it defers to the underlying Stream --- tests/SharpCompress.Test/Mocks/TestStream.cs | 101 +++++++++---------- 1 file changed, 50 insertions(+), 51 deletions(-) diff --git a/tests/SharpCompress.Test/Mocks/TestStream.cs b/tests/SharpCompress.Test/Mocks/TestStream.cs index d3f741c6..64bee443 100644 --- a/tests/SharpCompress.Test/Mocks/TestStream.cs +++ b/tests/SharpCompress.Test/Mocks/TestStream.cs @@ -2,69 +2,68 @@ namespace SharpCompress.Test.Mocks { - public class TestStream : Stream - { - private readonly Stream stream; + public class TestStream : Stream + { + private readonly Stream stream; - public TestStream(Stream stream) - : this(stream, true, true, true) - { - } + public TestStream(Stream stream) : this(stream, stream.CanRead, stream.CanWrite, stream.CanSeek) + { + } - public bool IsDisposed { get; private set; } + public bool IsDisposed { get; private set; } - public TestStream(Stream stream, bool read, bool write, bool seek) - { - this.stream = stream; - CanRead = read; - CanWrite = write; - CanSeek = seek; - } + public TestStream(Stream stream, bool read, bool write, bool seek) + { + this.stream = stream; + CanRead = read; + CanWrite = write; + CanSeek = seek; + } - protected override void Dispose(bool disposing) - { - base.Dispose(disposing); - stream.Dispose(); - IsDisposed = true; - } + protected override void Dispose(bool disposing) + { + base.Dispose(disposing); + stream.Dispose(); + IsDisposed = true; + } - public override bool CanRead { get; } + public override bool CanRead { get; } - public override bool CanSeek { get; } + public override bool CanSeek { get; } - public override bool CanWrite { get; } + public override bool CanWrite { get; } - public override void Flush() - { - stream.Flush(); - } + public override void Flush() + { + stream.Flush(); + } - public override long Length => stream.Length; + public override long Length => stream.Length; - public override long Position - { - get => stream.Position; - set => stream.Position = value; - } + public override long Position + { + get => stream.Position; + set => stream.Position = value; + } - public override int Read(byte[] buffer, int offset, int count) - { - return stream.Read(buffer, offset, count); - } + public override int Read(byte[] buffer, int offset, int count) + { + return stream.Read(buffer, offset, count); + } - public override long Seek(long offset, SeekOrigin origin) - { - return stream.Seek(offset, origin); - } + public override long Seek(long offset, SeekOrigin origin) + { + return stream.Seek(offset, origin); + } - public override void SetLength(long value) - { - stream.SetLength(value); - } + public override void SetLength(long value) + { + stream.SetLength(value); + } - public override void Write(byte[] buffer, int offset, int count) - { - stream.Write(buffer, offset, count); - } - } + public override void Write(byte[] buffer, int offset, int count) + { + stream.Write(buffer, offset, count); + } + } } From 44dc36af48ad7f834cde3855aac5110f5c9d4aed Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 15:34:57 -0700 Subject: [PATCH 15/23] Update ReaderTests base class to validate Dispose Update the `ReaderTests` base class to validate that `Dispose` is called appropriately in both the close and the leave open cases. --- tests/SharpCompress.Test/ReaderTests.cs | 28 +++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/tests/SharpCompress.Test/ReaderTests.cs b/tests/SharpCompress.Test/ReaderTests.cs index 1fb6d865..978a3ad1 100644 --- a/tests/SharpCompress.Test/ReaderTests.cs +++ b/tests/SharpCompress.Test/ReaderTests.cs @@ -13,11 +13,31 @@ namespace SharpCompress.Test { testArchive = Path.Combine(TEST_ARCHIVES_PATH, testArchive); - using (var stream = new NonDisposingStream(new ForwardOnlyStream(File.OpenRead(testArchive)), true)) - using (var reader = ReaderFactory.Open(stream, new ReaderOptions { LeaveStreamOpen = true })) + Read(testArchive, expectedCompression, true); + Read(testArchive, expectedCompression, false); + } + + private void Read(string testArchive, CompressionType expectedCompression, bool leaveStreamOpen) + { + using (var file = File.OpenRead(testArchive)) { - UseReader(this, reader, expectedCompression); - stream.ThrowOnDispose = false; + using (var protectedStream = new NonDisposingStream(new ForwardOnlyStream(file), throwOnDispose: true)) + { + using (var testStream = new TestStream(protectedStream)) + { + using (var reader = ReaderFactory.Open(testStream, new ReaderOptions { LeaveStreamOpen = leaveStreamOpen })) + { + UseReader(this, reader, expectedCompression); + protectedStream.ThrowOnDispose = false; + Assert.False(testStream.IsDisposed, "{nameof(testStream)} prematurely closed"); + } + + // Boolean XOR -- If the stream should be left open (true), then the stream should not be diposed (false) + // and if the stream should be closed (false), then the stream should be disposed (true) + var message = $"{nameof(leaveStreamOpen)} is set to '{leaveStreamOpen}', so {nameof(testStream.IsDisposed)} should be set to '{!testStream.IsDisposed}', but is set to {testStream.IsDisposed}"; + Assert.True(leaveStreamOpen != testStream.IsDisposed, message); + } + } } } From 7d20ba52438ad0a43cec0f52fff62de192ceee36 Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 16:16:28 -0700 Subject: [PATCH 16/23] Simplify RarHeaderTests --- .../SharpCompress.Test/Rar/RarHeaderFactoryTest.cs | 7 +------ tests/SharpCompress.Test/Rar/RarReaderTests.cs | 14 +------------- 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/tests/SharpCompress.Test/Rar/RarHeaderFactoryTest.cs b/tests/SharpCompress.Test/Rar/RarHeaderFactoryTest.cs index 283c7607..0c6e66a6 100644 --- a/tests/SharpCompress.Test/Rar/RarHeaderFactoryTest.cs +++ b/tests/SharpCompress.Test/Rar/RarHeaderFactoryTest.cs @@ -46,7 +46,7 @@ namespace SharpCompress.Test.Rar private void ReadEncryptedFlag(string testArchive, bool isEncrypted) { - using (var stream = GetReaderStream(testArchive)) + using (var stream = new FileStream(Path.Combine(TEST_ARCHIVES_PATH, testArchive), FileMode.Open, FileAccess.Read)) { foreach (var header in rarHeaderFactory.ReadHeaders(stream)) { @@ -58,10 +58,5 @@ namespace SharpCompress.Test.Rar } } } - - private FileStream GetReaderStream(string testArchive) - { - return new FileStream(Path.Combine(TEST_ARCHIVES_PATH, testArchive), FileMode.Open, FileAccess.Read); - } } } diff --git a/tests/SharpCompress.Test/Rar/RarReaderTests.cs b/tests/SharpCompress.Test/Rar/RarReaderTests.cs index 5b95ed94..e0a2a30f 100644 --- a/tests/SharpCompress.Test/Rar/RarReaderTests.cs +++ b/tests/SharpCompress.Test/Rar/RarReaderTests.cs @@ -205,20 +205,8 @@ namespace SharpCompress.Test.Rar Password = password })) { - while (reader.MoveToNextEntry()) - { - if (!reader.Entry.IsDirectory) - { - Assert.Equal(CompressionType.Rar, reader.Entry.CompressionType); - reader.WriteEntryToDirectory(SCRATCH_FILES_PATH, new ExtractionOptions() - { - ExtractFullPath = true, - Overwrite = true - }); - } - } + UseReader(this, reader, CompressionType.Rar); } - VerifyFiles(); } [Fact] From 933ffe7828362027bbc639b7ece207d638974e42 Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 16:33:46 -0700 Subject: [PATCH 17/23] Remove unused code from ArchiveTests --- tests/SharpCompress.Test/ArchiveTests.cs | 120 +++++------------------ 1 file changed, 23 insertions(+), 97 deletions(-) diff --git a/tests/SharpCompress.Test/ArchiveTests.cs b/tests/SharpCompress.Test/ArchiveTests.cs index c82c005c..716005fe 100644 --- a/tests/SharpCompress.Test/ArchiveTests.cs +++ b/tests/SharpCompress.Test/ArchiveTests.cs @@ -104,116 +104,42 @@ namespace SharpCompress.Test protected void ArchiveFileRead(string testArchive, ReaderOptions readerOptions = null) { testArchive = Path.Combine(TEST_ARCHIVES_PATH, testArchive); - ArchiveFileRead(testArchive.AsEnumerable(), readerOptions); - } - - protected void ArchiveFileRead(IEnumerable testArchives, ReaderOptions readerOptions = null) - { - foreach (var path in testArchives) + using (var archive = ArchiveFactory.Open(testArchive, readerOptions)) { - using (var archive = ArchiveFactory.Open(path, readerOptions)) + foreach (var entry in archive.Entries.Where(entry => !entry.IsDirectory)) { - //archive.EntryExtractionBegin += archive_EntryExtractionBegin; - //archive.FilePartExtractionBegin += archive_FilePartExtractionBegin; - //archive.CompressedBytesRead += archive_CompressedBytesRead; - - foreach (var entry in archive.Entries.Where(entry => !entry.IsDirectory)) - { - entry.WriteToDirectory(SCRATCH_FILES_PATH, - new ExtractionOptions() - { - ExtractFullPath = true, - Overwrite = true - }); - } + entry.WriteToDirectory(SCRATCH_FILES_PATH, + new ExtractionOptions() + { + ExtractFullPath = true, + Overwrite = true + }); } - VerifyFiles(); } + VerifyFiles(); } - private void archive_CompressedBytesRead(object sender, CompressedBytesReadEventArgs e) - { - Console.WriteLine("Read Compressed File Part Bytes: {0} Percentage: {1}%", - e.CurrentFilePartCompressedBytesRead, CreatePercentage(e.CurrentFilePartCompressedBytesRead, partTotal)); - - string percentage = entryTotal.HasValue ? CreatePercentage(e.CompressedBytesRead, - entryTotal.Value).ToString() : "Unknown"; - Console.WriteLine("Read Compressed File Entry Bytes: {0} Percentage: {1}%", - e.CompressedBytesRead, percentage); - } - - private void archive_FilePartExtractionBegin(object sender, FilePartExtractionBeginEventArgs e) - { - partTotal = e.Size; - Console.WriteLine("Initializing File Part Extraction: " + e.Name); - } - - private void archive_EntryExtractionBegin(object sender, ArchiveExtractionEventArgs e) - { - entryTotal = e.Item.Size; - Console.WriteLine("Initializing File Entry Extraction: " + e.Item.Key); - } - - private long? entryTotal; - private long partTotal; - private long totalSize; - + /// + /// Demonstrate the ExtractionOptions.PreserveFileTime and ExtractionOptions.PreserveAttributes extract options + /// protected void ArchiveFileReadEx(string testArchive) { testArchive = Path.Combine(TEST_ARCHIVES_PATH, testArchive); - ArchiveFileReadEx(testArchive.AsEnumerable()); - } - - /// - /// Demonstrate the TotalUncompressSize property, and the ExtractionOptions.PreserveFileTime and ExtractionOptions.PreserveAttributes extract options - /// - protected void ArchiveFileReadEx(IEnumerable testArchives) - { - foreach (var path in testArchives) + using (var archive = ArchiveFactory.Open(testArchive)) { - using (var archive = ArchiveFactory.Open(path)) + foreach (var entry in archive.Entries.Where(entry => !entry.IsDirectory)) { - totalSize = archive.TotalUncompressSize; - //archive.EntryExtractionBegin += Archive_EntryExtractionBeginEx; - //archive.EntryExtractionEnd += Archive_EntryExtractionEndEx; - //archive.CompressedBytesRead += Archive_CompressedBytesReadEx; - - foreach (var entry in archive.Entries.Where(entry => !entry.IsDirectory)) - { - entry.WriteToDirectory(SCRATCH_FILES_PATH, - new ExtractionOptions() - { - ExtractFullPath = true, - Overwrite = true, - PreserveAttributes = true, - PreserveFileTime = true - }); - } + entry.WriteToDirectory(SCRATCH_FILES_PATH, + new ExtractionOptions() + { + ExtractFullPath = true, + Overwrite = true, + PreserveAttributes = true, + PreserveFileTime = true + }); } - VerifyFilesEx(); } - } - - private void Archive_EntryExtractionEndEx(object sender, ArchiveExtractionEventArgs e) - { - partTotal += e.Item.Size; - } - - private void Archive_CompressedBytesReadEx(object sender, CompressedBytesReadEventArgs e) - { - string percentage = entryTotal.HasValue ? CreatePercentage(e.CompressedBytesRead, entryTotal.Value).ToString() : "-"; - string tortalPercentage = CreatePercentage(partTotal + e.CompressedBytesRead, totalSize).ToString(); - Console.WriteLine(@"Read Compressed File Progress: {0}% Total Progress {1}%", percentage, tortalPercentage); - } - - private void Archive_EntryExtractionBeginEx(object sender, ArchiveExtractionEventArgs e) - { - entryTotal = e.Item.Size; - } - - private int CreatePercentage(long n, long d) - { - return (int)(((double)n / (double)d) * 100); + VerifyFilesEx(); } } } From 3c2f4ebe9bf46d060dfe5f094f89799311d1201c Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 16:42:03 -0700 Subject: [PATCH 18/23] Combine ForwardOnlyStream and NonSeekableStream Delete `NonSeekableStream` used in Zip64 tests in favor of `ForwardOnlyStream` used in Mocks. Additionally, delete the `ForwardOnlyStream.ReadByte` implementation as the implementation on the base Stream is sufficient. --- .../Mocks/ForwardOnlyStream.cs | 5 --- tests/SharpCompress.Test/Zip/Zip64Tests.cs | 36 ++----------------- 2 files changed, 2 insertions(+), 39 deletions(-) diff --git a/tests/SharpCompress.Test/Mocks/ForwardOnlyStream.cs b/tests/SharpCompress.Test/Mocks/ForwardOnlyStream.cs index b5e5bbf1..aaa52c51 100644 --- a/tests/SharpCompress.Test/Mocks/ForwardOnlyStream.cs +++ b/tests/SharpCompress.Test/Mocks/ForwardOnlyStream.cs @@ -50,11 +50,6 @@ namespace SharpCompress.Test.Mocks return stream.Read(buffer, offset, count); } - public override int ReadByte() - { - return stream.ReadByte(); - } - public override long Seek(long offset, SeekOrigin origin) { throw new NotSupportedException(); diff --git a/tests/SharpCompress.Test/Zip/Zip64Tests.cs b/tests/SharpCompress.Test/Zip/Zip64Tests.cs index 45725a01..bac9a794 100644 --- a/tests/SharpCompress.Test/Zip/Zip64Tests.cs +++ b/tests/SharpCompress.Test/Zip/Zip64Tests.cs @@ -5,6 +5,7 @@ using SharpCompress.Archives; using SharpCompress.Common; using SharpCompress.Readers; using SharpCompress.Readers.Zip; +using SharpCompress.Test.Mocks; using SharpCompress.Writers; using SharpCompress.Writers.Zip; using Xunit; @@ -133,7 +134,7 @@ namespace SharpCompress.Test.Zip var eo = new ZipWriterEntryOptions() { DeflateCompressionLevel = Compressors.Deflate.CompressionLevel.None }; using (var zip = File.OpenWrite(filename)) - using(var st = forward_only ? (Stream)new NonSeekableStream(zip) : zip) + using(var st = forward_only ? (Stream)new ForwardOnlyStream(zip) : zip) using (var zipWriter = (ZipWriter)WriterFactory.Open(st, ArchiveType.Zip, opts)) { @@ -186,38 +187,5 @@ namespace SharpCompress.Test.Zip ); } } - - /// - /// Helper to create non-seekable streams from filestream - /// - private class NonSeekableStream : Stream - { - private readonly Stream stream; - public NonSeekableStream(Stream s) { stream = s; } - public override bool CanRead => stream.CanRead; - public override bool CanSeek => false; - public override bool CanWrite => stream.CanWrite; - public override long Length => throw new NotImplementedException(); - public override long Position { get => throw new NotImplementedException(); set => throw new NotImplementedException(); } - public override void Flush() { stream.Flush(); } - - public override int Read(byte[] buffer, int offset, int count) - { return stream.Read(buffer, offset, count); } - - public override int ReadByte() - { return stream.ReadByte(); } - - public override long Seek(long offset, SeekOrigin origin) - { throw new NotImplementedException(); } - - public override void SetLength(long value) - { throw new NotImplementedException(); } - - public override void Write(byte[] buffer, int offset, int count) - { stream.Write(buffer, offset, count); } - - public override void WriteByte(byte value) - { stream.WriteByte(value); } - } } } \ No newline at end of file From 87a14403820b6145b816921671ac3a5a3226b233 Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 16:49:49 -0700 Subject: [PATCH 19/23] Decouple UseReader from VerifyFiles --- tests/SharpCompress.Test/ReaderTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/SharpCompress.Test/ReaderTests.cs b/tests/SharpCompress.Test/ReaderTests.cs index 978a3ad1..68303c5f 100644 --- a/tests/SharpCompress.Test/ReaderTests.cs +++ b/tests/SharpCompress.Test/ReaderTests.cs @@ -15,6 +15,7 @@ namespace SharpCompress.Test Read(testArchive, expectedCompression, true); Read(testArchive, expectedCompression, false); + VerifyFiles(); } private void Read(string testArchive, CompressionType expectedCompression, bool leaveStreamOpen) @@ -55,7 +56,6 @@ namespace SharpCompress.Test }); } } - test.VerifyFiles(); } } } From e9a6fed6079e8b7e98fbdc4768634ce84d62cc0a Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 16:53:34 -0700 Subject: [PATCH 20/23] FIXUP decouple UseReader from VerifyFiles --- tests/SharpCompress.Test/ArchiveTests.cs | 4 ++-- tests/SharpCompress.Test/Rar/RarReaderTests.cs | 2 +- tests/SharpCompress.Test/ReaderTests.cs | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/SharpCompress.Test/ArchiveTests.cs b/tests/SharpCompress.Test/ArchiveTests.cs index 716005fe..c75aab72 100644 --- a/tests/SharpCompress.Test/ArchiveTests.cs +++ b/tests/SharpCompress.Test/ArchiveTests.cs @@ -10,7 +10,7 @@ using Xunit; namespace SharpCompress.Test { - public class ArchiveTests : TestBase + public class ArchiveTests : ReaderTests { protected void ArchiveStreamReadExtractAll(string testArchive, CompressionType compression) { @@ -29,7 +29,7 @@ namespace SharpCompress.Test Assert.True(archive.IsSolid); using (var reader = archive.ExtractAllEntries()) { - ReaderTests.UseReader(this, reader, compression); + UseReader(reader, compression); } VerifyFiles(); diff --git a/tests/SharpCompress.Test/Rar/RarReaderTests.cs b/tests/SharpCompress.Test/Rar/RarReaderTests.cs index e0a2a30f..a4119f22 100644 --- a/tests/SharpCompress.Test/Rar/RarReaderTests.cs +++ b/tests/SharpCompress.Test/Rar/RarReaderTests.cs @@ -205,7 +205,7 @@ namespace SharpCompress.Test.Rar Password = password })) { - UseReader(this, reader, CompressionType.Rar); + UseReader(reader, CompressionType.Rar); } } diff --git a/tests/SharpCompress.Test/ReaderTests.cs b/tests/SharpCompress.Test/ReaderTests.cs index 68303c5f..1d8e2bfb 100644 --- a/tests/SharpCompress.Test/ReaderTests.cs +++ b/tests/SharpCompress.Test/ReaderTests.cs @@ -28,7 +28,7 @@ namespace SharpCompress.Test { using (var reader = ReaderFactory.Open(testStream, new ReaderOptions { LeaveStreamOpen = leaveStreamOpen })) { - UseReader(this, reader, expectedCompression); + UseReader(reader, expectedCompression); protectedStream.ThrowOnDispose = false; Assert.False(testStream.IsDisposed, "{nameof(testStream)} prematurely closed"); } @@ -42,14 +42,14 @@ namespace SharpCompress.Test } } - public static void UseReader(TestBase test, IReader reader, CompressionType expectedCompression) + public void UseReader(IReader reader, CompressionType expectedCompression) { while (reader.MoveToNextEntry()) { if (!reader.Entry.IsDirectory) { Assert.Equal(reader.Entry.CompressionType, expectedCompression); - reader.WriteEntryToDirectory(test.SCRATCH_FILES_PATH, new ExtractionOptions() + reader.WriteEntryToDirectory(SCRATCH_FILES_PATH, new ExtractionOptions() { ExtractFullPath = true, Overwrite = true From 138038b08f165867257dbe353e5d9ed2b2d2b2d1 Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 18:37:49 -0700 Subject: [PATCH 21/23] Move RarReaderTests over to user ReaderFactory - Refactor `RarReaderTests` to use `ReaderFactory` - Update `ReaderTests.Read` to support Rar tests --- .../SharpCompress.Test/Rar/RarReaderTests.cs | 17 ++++---------- tests/SharpCompress.Test/ReaderTests.cs | 23 +++++++++++-------- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/tests/SharpCompress.Test/Rar/RarReaderTests.cs b/tests/SharpCompress.Test/Rar/RarReaderTests.cs index a4119f22..caf2caf6 100644 --- a/tests/SharpCompress.Test/Rar/RarReaderTests.cs +++ b/tests/SharpCompress.Test/Rar/RarReaderTests.cs @@ -199,14 +199,7 @@ namespace SharpCompress.Test.Rar private void ReadRar(string testArchive, string password) { - using (Stream stream = File.OpenRead(Path.Combine(TEST_ARCHIVES_PATH, testArchive))) - using (var reader = RarReader.Open(stream, new ReaderOptions() - { - Password = password - })) - { - UseReader(reader, CompressionType.Rar); - } + Read(testArchive, CompressionType.Rar, new ReaderOptions { Password = password }); } [Fact] @@ -222,7 +215,7 @@ namespace SharpCompress.Test.Rar private void DoRar_Entry_Stream(string filename) { using (Stream stream = File.OpenRead(Path.Combine(TEST_ARCHIVES_PATH, filename))) - using (var reader = RarReader.Open(stream)) + using (var reader = ReaderFactory.Open(stream)) { while (reader.MoveToNextEntry()) { @@ -255,7 +248,7 @@ namespace SharpCompress.Test.Rar public void Rar_Reader_Audio_program() { using (var stream = File.OpenRead(Path.Combine(TEST_ARCHIVES_PATH, "Rar.Audio_program.rar"))) - using (var reader = RarReader.Open(stream, new ReaderOptions() + using (var reader = ReaderFactory.Open(stream, new ReaderOptions() { LookForHeader = true })) @@ -321,7 +314,7 @@ namespace SharpCompress.Test.Rar private void DoRar_Solid_Skip_Reader(string filename) { using (var stream = File.OpenRead(Path.Combine(TEST_ARCHIVES_PATH, filename))) - using (var reader = RarReader.Open(stream, new ReaderOptions() + using (var reader = ReaderFactory.Open(stream, new ReaderOptions() { LookForHeader = true })) @@ -354,7 +347,7 @@ namespace SharpCompress.Test.Rar private void DoRar_Reader_Skip(string filename) { using (var stream = File.OpenRead(Path.Combine(TEST_ARCHIVES_PATH, filename))) - using (var reader = RarReader.Open(stream, new ReaderOptions() + using (var reader = ReaderFactory.Open(stream, new ReaderOptions() { LookForHeader = true })) diff --git a/tests/SharpCompress.Test/ReaderTests.cs b/tests/SharpCompress.Test/ReaderTests.cs index 1d8e2bfb..82ca6d29 100644 --- a/tests/SharpCompress.Test/ReaderTests.cs +++ b/tests/SharpCompress.Test/ReaderTests.cs @@ -7,18 +7,23 @@ using Xunit; namespace SharpCompress.Test { - public class ReaderTests : TestBase + public abstract class ReaderTests : TestBase { - protected void Read(string testArchive, CompressionType expectedCompression) + protected void Read(string testArchive, CompressionType expectedCompression, ReaderOptions options = null) { testArchive = Path.Combine(TEST_ARCHIVES_PATH, testArchive); - Read(testArchive, expectedCompression, true); - Read(testArchive, expectedCompression, false); + options = options ?? new ReaderOptions(); + + options.LeaveStreamOpen = true; + ReadImpl(testArchive, expectedCompression, options); + + options.LeaveStreamOpen = false; + ReadImpl(testArchive, expectedCompression, options); VerifyFiles(); } - private void Read(string testArchive, CompressionType expectedCompression, bool leaveStreamOpen) + private void ReadImpl(string testArchive, CompressionType expectedCompression, ReaderOptions options) { using (var file = File.OpenRead(testArchive)) { @@ -26,7 +31,7 @@ namespace SharpCompress.Test { using (var testStream = new TestStream(protectedStream)) { - using (var reader = ReaderFactory.Open(testStream, new ReaderOptions { LeaveStreamOpen = leaveStreamOpen })) + using (var reader = ReaderFactory.Open(testStream, options)) { UseReader(reader, expectedCompression); protectedStream.ThrowOnDispose = false; @@ -35,8 +40,8 @@ namespace SharpCompress.Test // Boolean XOR -- If the stream should be left open (true), then the stream should not be diposed (false) // and if the stream should be closed (false), then the stream should be disposed (true) - var message = $"{nameof(leaveStreamOpen)} is set to '{leaveStreamOpen}', so {nameof(testStream.IsDisposed)} should be set to '{!testStream.IsDisposed}', but is set to {testStream.IsDisposed}"; - Assert.True(leaveStreamOpen != testStream.IsDisposed, message); + var message = $"{nameof(options.LeaveStreamOpen)} is set to '{options.LeaveStreamOpen}', so {nameof(testStream.IsDisposed)} should be set to '{!testStream.IsDisposed}', but is set to {testStream.IsDisposed}"; + Assert.True(options.LeaveStreamOpen != testStream.IsDisposed, message); } } } @@ -48,7 +53,7 @@ namespace SharpCompress.Test { if (!reader.Entry.IsDirectory) { - Assert.Equal(reader.Entry.CompressionType, expectedCompression); + Assert.Equal(expectedCompression, reader.Entry.CompressionType); reader.WriteEntryToDirectory(SCRATCH_FILES_PATH, new ExtractionOptions() { ExtractFullPath = true, From 93c0b91de9b628ae2ca82043d2cdec767c312bee Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 18:52:33 -0700 Subject: [PATCH 22/23] Refactor TestSharpCompressWithEmptyStream Refactor `TestSharpCompressWithEmptyStream` so it asserts that the files and bytes are the same. --- .../SharpCompress.Test/Zip/ZipReaderTests.cs | 56 +++++++++++-------- 1 file changed, 32 insertions(+), 24 deletions(-) diff --git a/tests/SharpCompress.Test/Zip/ZipReaderTests.cs b/tests/SharpCompress.Test/Zip/ZipReaderTests.cs index 2da74330..9c24ad56 100644 --- a/tests/SharpCompress.Test/Zip/ZipReaderTests.cs +++ b/tests/SharpCompress.Test/Zip/ZipReaderTests.cs @@ -269,40 +269,48 @@ namespace SharpCompress.Test.Zip VerifyFiles(); } - private class NonSeekableMemoryStream : MemoryStream - { - public override bool CanSeek => false; - } - [Fact] public void TestSharpCompressWithEmptyStream() { - - MemoryStream stream = new NonSeekableMemoryStream(); - - using (IWriter zipWriter = WriterFactory.Open(stream, ArchiveType.Zip, CompressionType.Deflate)) + var expected = new Tuple[] { - zipWriter.Write("foo.txt", new MemoryStream(new byte[0])); - zipWriter.Write("foo2.txt", new MemoryStream(new byte[10])); - } + new Tuple("foo.txt", new byte[0]), + new Tuple("foo2.txt", new byte[10]) + }; - stream = new MemoryStream(stream.ToArray()); - File.WriteAllBytes(Path.Combine(SCRATCH_FILES_PATH, "foo.zip"), stream.ToArray()); - - using (IReader zipReader = ZipReader.Open(new NonDisposingStream(stream, true))) + using (var memory = new MemoryStream()) { - while (zipReader.MoveToNextEntry()) + Stream stream = new TestStream(memory, read: true, write: true, seek: false); + + using (IWriter zipWriter = WriterFactory.Open(stream, ArchiveType.Zip, CompressionType.Deflate)) { - using (EntryStream entry = zipReader.OpenEntryStream()) + zipWriter.Write(expected[0].Item1, new MemoryStream(expected[0].Item2)); + zipWriter.Write(expected[1].Item1, new MemoryStream(expected[1].Item2)); + } + + stream = new MemoryStream(memory.ToArray()); + File.WriteAllBytes(Path.Combine(SCRATCH_FILES_PATH, "foo.zip"), memory.ToArray()); + + using (IReader zipReader = ZipReader.Open(new NonDisposingStream(stream, true))) + { + var i = 0; + while (zipReader.MoveToNextEntry()) { - MemoryStream tempStream = new MemoryStream(); - const int bufSize = 0x1000; - byte[] buf = new byte[bufSize]; - int bytesRead = 0; - while ((bytesRead = entry.Read(buf, 0, bufSize)) > 0) + using (EntryStream entry = zipReader.OpenEntryStream()) { - tempStream.Write(buf, 0, bytesRead); + MemoryStream tempStream = new MemoryStream(); + const int bufSize = 0x1000; + byte[] buf = new byte[bufSize]; + int bytesRead = 0; + while ((bytesRead = entry.Read(buf, 0, bufSize)) > 0) + { + tempStream.Write(buf, 0, bytesRead); + } + + Assert.Equal(expected[i].Item1, zipReader.Entry.Key); + Assert.Equal(expected[i].Item2, tempStream.ToArray()); } + i++; } } } From f7ad5959456e40429691db6bb064dfb94af364ed Mon Sep 17 00:00:00 2001 From: Matt Kotsenas Date: Wed, 11 Jul 2018 17:48:45 -0700 Subject: [PATCH 23/23] Enable test parallelization and remove garbage collection workaround Now that the sources of file locking are fixed, enable test parallelization and the forced garbage collection workaround. Lastly, remove the `IsLocked` check because it doesn't work in a parallel test running world - the file may be locked due to another test running. --- tests/SharpCompress.Test/TestBase.cs | 40 ---------------------------- 1 file changed, 40 deletions(-) diff --git a/tests/SharpCompress.Test/TestBase.cs b/tests/SharpCompress.Test/TestBase.cs index da73fab6..3267e325 100644 --- a/tests/SharpCompress.Test/TestBase.cs +++ b/tests/SharpCompress.Test/TestBase.cs @@ -6,8 +6,6 @@ using System.Text; using SharpCompress.Readers; using Xunit; -[assembly: CollectionBehavior(DisableTestParallelization = true)] - namespace SharpCompress.Test { public class TestBase : IDisposable @@ -39,12 +37,6 @@ namespace SharpCompress.Test public void Dispose() { - // WARNING: This garbage collection is needed to reclaim leaked file handles - // (likely due to improperly handled IDisposables). Without it, The following - // delete fails because files are still is use. This GC should be removed once - // all the files pass without it. - GC.Collect(2, GCCollectionMode.Forced, true, false); - Directory.Delete(SCRATCH_BASE_PATH, true); } @@ -169,14 +161,6 @@ namespace SharpCompress.Test return; } - if (IsFileLocked(new FileInfo(file1))) - { - throw new InvalidOperationException($"{file1} is not disposed"); - } - if (IsFileLocked(new FileInfo(file2))) - { - throw new InvalidOperationException($"{file2} is not disposed"); - } using (var file1Stream = File.OpenRead(file1)) using (var file2Stream = File.OpenRead(file2)) { @@ -230,29 +214,5 @@ namespace SharpCompress.Test } } - protected bool IsFileLocked(FileInfo file) - { - FileStream stream = null; - - try - { - stream = file.Open(FileMode.Open, FileAccess.Read, FileShare.None); - } - catch (IOException) - { - //the file is unavailable because it is: - //still being written to - //or being processed by another thread - //or does not exist (has already been processed) - return true; - } - finally - { - stream?.Close(); - } - - //file is not locked - return false; - } } }