-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix ZipArchive Update mode rewriting unchanged archives on Dispose #123719
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
|
Tagging subscribers to this area: @karelz, @dotnet/area-system-io-compression |
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
Fixes a regression where opening entry streams in ZipArchiveMode.Update could mark the archive as modified even when no data was written, causing unnecessary rewrites on Dispose (and failures on non-expandable streams).
Changes:
- Track actual writes in Update mode by notifying the entry only on the first write to the opened stream.
- Skip rewriting Update-mode archives on
Disposeunless the archive is actually modified (while still writing newly-created empty archives). - Add regression tests covering Package/Zip scenarios where entries are read without writes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.IO.Packaging/tests/Tests.cs | Adds regression test ensuring Package.Open(ReadWrite) doesn’t throw on Dispose when only reading. |
| src/libraries/System.IO.Compression/tests/ZipArchive/zip_UpdateTests.cs | Updates/extends Update-mode tests and adds regression tests for non-expandable streams. |
| src/libraries/System.IO.Compression/src/System/IO/Compression/ZipCustomStreams.cs | Adds write-notification plumbing to mark entries modified only when written. |
| src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs | Stops marking entries modified on open; introduces MarkAsModified; adjusts write path to reuse original compressed bytes if unchanged. |
| src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.Async.cs | Async equivalents of Update-mode changes and unchanged-data handling. |
| src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs | Adds Update-mode “write only if modified” behavior and “new archive” detection. |
| src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.Async.cs | Async equivalents of Update-mode “write only if modified” and “new archive” detection. |
src/libraries/System.IO.Compression/tests/ZipArchive/zip_UpdateTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.Async.cs
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs
Show resolved
Hide resolved
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
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
| /// <summary> | ||
| /// Marks this entry as modified, indicating that its data has changed and needs to be rewritten. | ||
| /// </summary> |
Copilot
AI
Jan 30, 2026
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 are two consecutive <summary> XML doc blocks above MarkAsModified(). This duplication is likely accidental and should be reduced to a single <summary> to avoid confusing generated docs / analyzers.
| /// <summary> | |
| /// Marks this entry as modified, indicating that its data has changed and needs to be rewritten. | |
| /// </summary> |
| // Only write if the archive has been modified | ||
| if (IsModified) | ||
| { | ||
| WriteFile(); | ||
| } |
Copilot
AI
Jan 30, 2026
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.
In Update mode, skipping WriteFile() when IsModified is false means entries that were opened (which loads _storedUncompressedData into memory) may never have their internal buffers disposed/unloaded. This can retain large MemoryStream buffers longer than necessary (especially if callers keep references to ZipArchiveEntry instances after disposing the archive). Consider explicitly unloading per-entry buffers during Dispose even when you don't rewrite (e.g., iterate entries and clear/dispose cached data).
| Debug.Assert(_mode == ZipArchiveMode.Update); | ||
| // Only write if the archive has been modified | ||
| if (IsModified) | ||
| { | ||
| await WriteFileAsync().ConfigureAwait(false); | ||
| } |
Copilot
AI
Jan 30, 2026
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.
Async Dispose has the same resource-retention concern as sync Dispose: when IsModified is false, WriteFileAsync() is skipped and per-entry cached buffers (like _storedUncompressedData) may never be disposed/unloaded. Consider explicitly unloading per-entry buffers during DisposeAsync even when you don't rewrite.
| // extract and check the file | ||
| zip = await CreateZipArchive(async, testStream, ZipArchiveMode.Update); | ||
|
|
||
| ZipArchiveEntry entry = zip.GetEntry(ExpectedFileName); | ||
| // The entry retains its original properties since it was never modified | ||
| Assert.Equal(0, entry.Length); |
Copilot
AI
Jan 30, 2026
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.
Before re-opening testStream for the final verification, the stream should be rewound to the beginning. CreateZipArchive does not reset the stream position, and after disposing the previous archive the stream is likely positioned near EOF, which can make the subsequent open/read behave incorrectly or fail.
Fix ZipArchive in Update mode rewriting when no changes made
Opening an entry stream in ZipArchiveMode.Update would immediately mark the archive as modified, causing Dispose to attempt a rewrite even when no actual writes occurred. This threw NotSupportedException on non-expandable streams like fixed-size MemoryStream.
The fix tracks actual writes via WrappedStream and only marks the entry as modified when data is written. Archives created on empty streams are marked as new to ensure they're still written on first creation.
Fixes #123419