Skip to content

Commit 2a92fc1

Browse files
ZingBallyhooZingBallyhoo
authored
Fix async ZipArchive calling non-async Stream methods (#121938)
Fixes #121624 Supersedes #121633 Note: this could be classed as a breaking change, as an exception that was thrown on accessing `.Entries` is now thrown from `CreateAsync` (although this is the approved API shape #1541 (comment)). The `ZipArchive_InvalidEntryTable` and `ZipFile_Open.InvalidFilesAsync` tests have been updated to reflect this.
1 parent 3b63772 commit 2a92fc1

File tree

6 files changed

+63
-157
lines changed

6 files changed

+63
-157
lines changed

‎src/libraries/Common/tests/System/IO/Compression/NoAsyncCallsStream.cs‎

Lines changed: 7 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,6 @@ internal sealed class NoAsyncCallsStream : Stream
1313

1414
public NoAsyncCallsStream(Stream stream) => _s = stream;
1515

16-
// Allows temporarily disabling the current stream's sync API usage restriction.
17-
public bool IsRestrictionEnabled { get; set; }
18-
1916
public override bool CanRead => _s.CanRead;
2017
public override bool CanSeek => _s.CanSeek;
2118
public override bool CanTimeout => _s.CanTimeout;
@@ -47,18 +44,11 @@ internal sealed class NoAsyncCallsStream : Stream
4744
public override void WriteByte(byte value) => _s.WriteByte(value);
4845

4946
// Async
50-
public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) =>
51-
IsRestrictionEnabled ? throw new InvalidOperationException() : _s.CopyToAsync(destination, bufferSize, cancellationToken);
52-
public override ValueTask DisposeAsync() =>
53-
IsRestrictionEnabled ? throw new InvalidOperationException() : _s.DisposeAsync();
54-
public override Task FlushAsync(CancellationToken cancellationToken) =>
55-
IsRestrictionEnabled ? throw new InvalidOperationException() : _s.FlushAsync();
56-
public override Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) =>
57-
IsRestrictionEnabled ? throw new InvalidOperationException() : _s.ReadAsync(buffer, offset, count, cancellationToken);
58-
public override ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default) =>
59-
IsRestrictionEnabled ? throw new InvalidOperationException() : _s.ReadAsync(buffer, cancellationToken);
60-
public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) =>
61-
IsRestrictionEnabled ? throw new InvalidOperationException() : _s.WriteAsync(buffer, offset, count, cancellationToken);
62-
public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken = default) =>
63-
IsRestrictionEnabled ? throw new InvalidOperationException() : _s.WriteAsync(buffer, cancellationToken);
47+
public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) => throw new InvalidOperationException();
48+
public override ValueTask DisposeAsync() => throw new InvalidOperationException();
49+
public override Task FlushAsync(CancellationToken cancellationToken) => throw new InvalidOperationException();
50+
public override Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) => throw new InvalidOperationException();
51+
public override ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default) => throw new InvalidOperationException();
52+
public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) => throw new InvalidOperationException();
53+
public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken = default) => throw new InvalidOperationException();
6454
}

‎src/libraries/Common/tests/System/IO/Compression/NoSyncCallsStream.cs‎

Lines changed: 12 additions & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,6 @@ internal sealed class NoSyncCallsStream : Stream
1313

1414
public NoSyncCallsStream(Stream stream) => _s = stream;
1515

16-
// Allows temporarily disabling the current stream's sync API usage restriction.
17-
public bool IsRestrictionEnabled { get; set; }
18-
1916
public override bool CanRead => _s.CanRead;
2017
public override bool CanSeek => _s.CanSeek;
2118
public override bool CanTimeout => _s.CanTimeout;
@@ -33,105 +30,18 @@ internal sealed class NoSyncCallsStream : Stream
3330
public override string? ToString() => _s.ToString();
3431

3532
// Sync
36-
public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) =>
37-
IsRestrictionEnabled ? throw new InvalidOperationException() : _s.BeginRead(buffer, offset, count, callback, state);
38-
public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) =>
39-
IsRestrictionEnabled ? throw new InvalidOperationException() : _s.BeginRead(buffer, offset, count, callback, state);
40-
public override void CopyTo(Stream destination, int bufferSize)
41-
{
42-
if (IsRestrictionEnabled)
43-
{
44-
throw new InvalidOperationException();
45-
}
46-
else
47-
{
48-
_s.CopyTo(destination, bufferSize);
49-
}
50-
}
51-
protected override void Dispose(bool disposing)
52-
{
53-
// _disposing = true;
54-
if (IsRestrictionEnabled)
55-
{
56-
throw new InvalidOperationException();
57-
}
58-
else
59-
{
60-
_s.Dispose();
61-
}
62-
}
63-
public override int EndRead(IAsyncResult asyncResult) => IsRestrictionEnabled ? throw new InvalidOperationException() : _s.EndRead(asyncResult);
64-
public override void EndWrite(IAsyncResult asyncResult)
65-
{
66-
if (IsRestrictionEnabled)
67-
{
68-
throw new InvalidOperationException();
69-
}
70-
else
71-
{
72-
_s.EndWrite(asyncResult);
73-
}
74-
}
75-
public override void Flush()
76-
{
77-
if (IsRestrictionEnabled)
78-
{
79-
throw new InvalidOperationException();
80-
}
81-
else
82-
{
83-
_s.Flush();
84-
}
85-
}
86-
public override int Read(byte[] buffer, int offset, int count) =>
87-
IsRestrictionEnabled ? throw new InvalidOperationException() : _s.Read(buffer, offset, count);
88-
public override int Read(Span<byte> buffer) =>
89-
IsRestrictionEnabled ? throw new InvalidOperationException() : _s.Read(buffer);
90-
public override void Write(byte[] buffer, int offset, int count)
91-
{
92-
bool isDeflateStream = false;
93-
94-
// Get the stack trace to determine the calling method
95-
var stackTrace = new System.Diagnostics.StackTrace();
96-
var callingMethod = stackTrace.GetFrame(1)?.GetMethod();
97-
98-
// Check if the calling method belongs to the DeflateStream class
99-
if (callingMethod?.DeclaringType == typeof(System.IO.Compression.DeflateStream))
100-
{
101-
isDeflateStream = true;
102-
}
103-
104-
if (!isDeflateStream && IsRestrictionEnabled)
105-
{
106-
throw new InvalidOperationException($"Parent class is {callingMethod.DeclaringType}");
107-
}
108-
else
109-
{
110-
_s.Write(buffer, offset, count);
111-
}
112-
}
113-
public override void Write(ReadOnlySpan<byte> buffer)
114-
{
115-
if (IsRestrictionEnabled)
116-
{
117-
throw new InvalidOperationException();
118-
}
119-
else
120-
{
121-
_s.Write(buffer);
122-
}
123-
}
124-
public override void WriteByte(byte value)
125-
{
126-
if (IsRestrictionEnabled)
127-
{
128-
throw new InvalidOperationException();
129-
}
130-
else
131-
{
132-
_s.WriteByte(value);
133-
}
134-
}
33+
public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) => throw new InvalidOperationException();
34+
public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) => throw new InvalidOperationException();
35+
public override void CopyTo(Stream destination, int bufferSize) => throw new InvalidOperationException();
36+
protected override void Dispose(bool disposing) => throw new InvalidOperationException();
37+
public override int EndRead(IAsyncResult asyncResult) => throw new InvalidOperationException();
38+
public override void EndWrite(IAsyncResult asyncResult) => throw new InvalidOperationException();
39+
public override void Flush() => throw new InvalidOperationException();
40+
public override int Read(byte[] buffer, int offset, int count) => throw new InvalidOperationException();
41+
public override int Read(Span<byte> buffer) => throw new InvalidOperationException();
42+
public override void Write(byte[] buffer, int offset, int count) => throw new InvalidOperationException();
43+
public override void Write(ReadOnlySpan<byte> buffer) => throw new InvalidOperationException();
44+
public override void WriteByte(byte value) => throw new InvalidOperationException();
13545

13646
// Async
13747
public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) => _s.CopyToAsync(destination, bufferSize, cancellationToken);

‎src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Open.cs‎

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -134,20 +134,13 @@ public async Task InvalidFilesAsync()
134134
await Assert.ThrowsAsync<InvalidDataException>(() => ZipFile.OpenAsync(testArchive.Path, ZipArchiveMode.Update, default));
135135
}
136136

137-
await using (ZipArchive archive = await ZipFile.OpenReadAsync(bad("CDoffsetInBoundsWrong.zip"), default))
138-
{
139-
Assert.Throws<InvalidDataException>(() => { var x = archive.Entries; });
140-
}
141-
137+
await Assert.ThrowsAsync<InvalidDataException>(() => ZipFile.OpenReadAsync(bad("CDoffsetInBoundsWrong.zip"), default));
142138
using (TempFile testArchive = CreateTempCopyFile(bad("CDoffsetInBoundsWrong.zip"), GetTestFilePath()))
143139
{
144140
await Assert.ThrowsAsync<InvalidDataException>(() => ZipFile.OpenAsync(testArchive.Path, ZipArchiveMode.Update, default));
145141
}
146142

147-
await using (ZipArchive archive = await ZipFile.OpenReadAsync(bad("numberOfEntriesDifferent.zip"), default))
148-
{
149-
Assert.Throws<InvalidDataException>(() => { var x = archive.Entries; });
150-
}
143+
await Assert.ThrowsAsync<InvalidDataException>(() => ZipFile.OpenReadAsync(bad("numberOfEntriesDifferent.zip"), default));
151144
using (TempFile testArchive = CreateTempCopyFile(bad("numberOfEntriesDifferent.zip"), GetTestFilePath()))
152145
{
153146
await Assert.ThrowsAsync<InvalidDataException>(() => ZipFile.OpenAsync(testArchive.Path, ZipArchiveMode.Update, default));

‎src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.Async.cs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,10 @@ public static async Task<ZipArchive> CreateAsync(Stream stream, ZipArchiveMode m
9292
break;
9393
case ZipArchiveMode.Read:
9494
await zipArchive.ReadEndOfCentralDirectoryAsync(cancellationToken).ConfigureAwait(false);
95+
96+
// As there is no API for accessing .Entries asynchronously, we are expected to read the central
97+
// directory up-front
98+
await zipArchive.EnsureCentralDirectoryReadAsync(cancellationToken).ConfigureAwait(false);
9599
break;
96100
case ZipArchiveMode.Update:
97101
default:

‎src/libraries/System.IO.Compression/src/System/IO/Compression/ZipCustomStreams.cs‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,20 @@ protected override void Dispose(bool disposing)
216216
}
217217
base.Dispose(disposing);
218218
}
219+
220+
public override async ValueTask DisposeAsync()
221+
{
222+
if (!_isDisposed)
223+
{
224+
_onClosed?.Invoke(_zipArchiveEntry);
225+
226+
if (_closeBaseStream)
227+
await _baseStream.DisposeAsync().ConfigureAwait(false);
228+
229+
_isDisposed = true;
230+
}
231+
await base.DisposeAsync().ConfigureAwait(false);
232+
}
219233
}
220234

221235
internal sealed class SubReadStream : Stream

‎src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs‎

Lines changed: 24 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -644,9 +644,20 @@ public static async Task ZipArchive_InvalidStream(string zipname, bool async)
644644
public static async Task ZipArchive_InvalidEntryTable(string zipname, bool async)
645645
{
646646
string filename = bad(zipname);
647-
await using (ZipArchive archive = await CreateZipArchive(async, await StreamHelpers.CreateTempCopyStream(filename), ZipArchiveMode.Read))
647+
using (var stream = await StreamHelpers.CreateTempCopyStream(filename))
648648
{
649-
Assert.Throws<InvalidDataException>(() => archive.Entries[0]);
649+
if (async)
650+
{
651+
// as CreateAsync reads the entry table immediately, it throws immediately instead of on accessing .Entries
652+
await Assert.ThrowsAsync<InvalidDataException>(() => CreateZipArchive(async, stream, ZipArchiveMode.Read));
653+
}
654+
else
655+
{
656+
await using (ZipArchive archive = await CreateZipArchive(async, stream, ZipArchiveMode.Read))
657+
{
658+
Assert.Throws<InvalidDataException>(() => archive.Entries[0]);
659+
}
660+
}
650661
}
651662
}
652663

@@ -1484,24 +1495,24 @@ public static async Task NoAsyncCallsWhenUsingSync()
14841495
// Create mode
14851496
using (ZipArchive archive = new ZipArchive(s, ZipArchiveMode.Create, leaveOpen: true, entryNameEncoding: Encoding.UTF8))
14861497
{
1487-
using MemoryStream normalZipStream = await StreamHelpers.CreateTempCopyStream(zfile("normal.zip"));
1488-
normalZipStream.Position = 0;
1498+
using MemoryStream asyncZipStream = await StreamHelpers.CreateTempCopyStream(zfile("normal.zip"));
1499+
asyncZipStream.Position = 0;
14891500

14901501
// Note this is not using NoAsyncCallsStream, so it can be opened in async mode
1491-
await using (ZipArchive normalZipArchive = await ZipArchive.CreateAsync(normalZipStream, ZipArchiveMode.Read, leaveOpen: false, entryNameEncoding: null))
1502+
await using (ZipArchive asyncZipArchive = await ZipArchive.CreateAsync(asyncZipStream, ZipArchiveMode.Read, leaveOpen: false, entryNameEncoding: null))
14921503
{
1493-
var normalZipEntries = normalZipArchive.Entries;
1504+
var asyncZipEntries = asyncZipArchive.Entries;
14941505

1495-
foreach (ZipArchiveEntry normalEntry in normalZipEntries)
1506+
foreach (ZipArchiveEntry asyncEntry in asyncZipEntries)
14961507
{
1497-
ZipArchiveEntry newEntry = archive.CreateEntry(normalEntry.FullName);
1508+
ZipArchiveEntry newEntry = archive.CreateEntry(asyncEntry.FullName);
14981509
using (Stream newEntryStream = newEntry.Open())
14991510
{
15001511
// Note the parent archive is not using NoAsyncCallsStream, so it can be opened in async mode
1501-
await using (Stream normalEntryStream = await normalEntry.OpenAsync())
1512+
await using (Stream asyncEntryStream = await asyncEntry.OpenAsync())
15021513
{
1503-
// Note the parent archive is not using NoAsyncCallsStream, so it can be copied in async mode
1504-
await normalEntryStream.CopyToAsync(newEntryStream);
1514+
// The entry needs to be copied in sync mode as the destination archive does not support async writes
1515+
asyncEntryStream.CopyTo(newEntryStream);
15051516
}
15061517
}
15071518
}
@@ -1515,11 +1526,7 @@ public static async Task NoAsyncCallsWhenUsingSync()
15151526
{
15161527
_ = archive.Comment;
15171528

1518-
// Entries is sync only
1519-
s.IsRestrictionEnabled = false;
15201529
var entries = archive.Entries;
1521-
s.IsRestrictionEnabled = true;
1522-
15231530
foreach (var entry in entries)
15241531
{
15251532
_ = archive.GetEntry(entry.Name);
@@ -1550,11 +1557,7 @@ public static async Task NoAsyncCallsWhenUsingSync()
15501557
// Update mode
15511558
using (ZipArchive archive = new ZipArchive(s, ZipArchiveMode.Update, leaveOpen: false, entryNameEncoding: Encoding.UTF8))
15521559
{
1553-
// Entries is sync only
1554-
s.IsRestrictionEnabled = false;
15551560
ZipArchiveEntry entryToDelete = archive.Entries[0];
1556-
s.IsRestrictionEnabled = true;
1557-
15581561
entryToDelete.Delete();
15591562

15601563
ZipArchiveEntry entry = archive.CreateEntry("mynewentry.txt");
@@ -1593,8 +1596,8 @@ public static async Task NoSyncCallsWhenUsingAsync()
15931596
// Note the parent archive is not using NoSyncCallsStream, so it can be opened in sync mode
15941597
using (Stream normalEntryStream = normalEntry.Open())
15951598
{
1596-
// Note the parent archive is not using NoSyncCallsStream, so it can be copied in sync mode
1597-
normalEntryStream.CopyTo(newEntryStream);
1599+
// The entry needs to be copied in async mode as the destination archive does not support sync writes
1600+
await normalEntryStream.CopyToAsync(newEntryStream);
15981601
}
15991602
}
16001603
}
@@ -1608,11 +1611,7 @@ public static async Task NoSyncCallsWhenUsingAsync()
16081611
{
16091612
_ = archive.Comment;
16101613

1611-
// Entries is sync only
1612-
s.IsRestrictionEnabled = false;
16131614
var entries = archive.Entries;
1614-
s.IsRestrictionEnabled = true;
1615-
16161615
foreach (var entry in entries)
16171616
{
16181617
_ = archive.GetEntry(entry.Name);
@@ -1642,11 +1641,7 @@ public static async Task NoSyncCallsWhenUsingAsync()
16421641

16431642
await using (ZipArchive archive = await ZipArchive.CreateAsync(s, ZipArchiveMode.Update, leaveOpen: false, entryNameEncoding: Encoding.UTF8))
16441643
{
1645-
// Entries is sync only
1646-
s.IsRestrictionEnabled = false;
16471644
ZipArchiveEntry entryToDelete = archive.Entries[0];
1648-
s.IsRestrictionEnabled = true;
1649-
16501645
entryToDelete.Delete(); // Delete is async only
16511646

16521647
ZipArchiveEntry entry = archive.CreateEntry("mynewentry.txt");

0 commit comments

Comments
 (0)