-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Add DisposeAsync to WrappedStream to prevent synchronous calls in async Zip operations #121633
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where asynchronous ZIP archive operations still made synchronous calls during disposal, causing failures with async-only streams like Kestrel response streams. The problem was that WrappedStream lacked a DisposeAsync override, causing await using statements to fall back to synchronous disposal.
- Added
DisposeAsyncoverride toWrappedStreamthat asynchronously disposes the base stream - Added comprehensive test with async-only stream wrapper to verify no synchronous calls occur during async disposal
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/System.IO.Compression/src/System/IO/Compression/ZipCustomStreams.cs | Added DisposeAsync method to WrappedStream class following the same pattern as CheckSumAndSizeWriteStream.DisposeAsync |
| src/libraries/System.IO.Compression/tests/ZipArchive/zip_netcoreappTests.cs | Added test AsyncOnlyStream_NoSynchronousCalls and helper class AsyncOnlyStream to verify async operations don't make synchronous calls |
| } | ||
|
|
||
| [Fact] | ||
| public static async Task AsyncOnlyStream_NoSynchronousCalls() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already a test for this, but it fails to validate that DisposeAsync has no sync calls due to a test bug
runtime/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs
Line 1572 in 4d8ec94
| public static async Task NoSyncCallsWhenUsingAsync() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the test does not assert the async-only execution because:
s.IsRestrictionEnabled is left to the default (false), same for NoAsync* version
runtime/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs
Lines 1479 to 1484 in dc24bfc
| public static async Task NoAsyncCallsWhenUsingSync() | |
| { | |
| using MemoryStream ms = new(); | |
| using NoAsyncCallsStream s = new(ms); // Only allows sync calls | |
| // Create mode |
And on top of it, it explicitly ignores violations if called from DeflateStream?!
runtime/src/libraries/Common/tests/System/IO/Compression/NoSyncCallsStream.cs
Lines 98 to 107 in dc24bfc
| // Check if the calling method belongs to the DeflateStream class | |
| if (callingMethod?.DeclaringType == typeof(System.IO.Compression.DeflateStream)) | |
| { | |
| isDeflateStream = true; | |
| } | |
| if (!isDeflateStream && IsRestrictionEnabled) | |
| { | |
| throw new InvalidOperationException($"Parent class is {callingMethod.DeclaringType}"); | |
| } |
I think we can safely remove that filtering.
Fixing those two places makes the new test redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Can you address my comment?
|
@copilot remove the new test and fix the existing one as per instructions in my comment |
I don't see the detailed instructions you're referring to in the comments provided. Could you please clarify which existing test needs to be fixed and what changes are needed? I want to make sure I address your feedback correctly. |
|
@copilot, let me repeat There is already a test for not using sync methods in async methods: zip_InvalidParametersAndStrangeFiles .NoSyncCallsWhenUsingAsync. That test needs to be fixed so that it tests the functionality properly: Make sure |
…eview feedback Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
Done in commit 2634862. I've:
The existing test now properly validates that no synchronous calls are made when using async methods. |
|
Now that we know the bypass for That would also mean fixing
I'm happy to PR this myself if Copilot can't be coerced to do it. |
|
Those sound like good improvements, we would gladly accept a contribution :) |
Fixes #121624
The async Zip methods added in #114421 still made synchronous calls during disposal, causing failures with async-only streams (e.g., Kestrel response streams). The
WrappedStreamclass lacked aDisposeAsyncoverride, soawait usingfell back to synchronousDispose(), which then made synchronous writes.Changes
DisposeAsyncoverride toWrappedStreamthat asynchronously disposes the base streamNoSyncCallsWhenUsingAsynctest to properly validate async-only behavior:IsRestrictionEnabled = trueonNoSyncCallsStreamto enforce async-only restrictionsNoSyncCallsStream.Write()to properly catch all synchronous callsCopyToAsyncinstead ofCopyTofor proper async operationExample
Follows the same pattern as
CheckSumAndSizeWriteStream.DisposeAsyncin the same file.💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.