fix: dispose source streams in writable archive async add entry#1310
fix: dispose source streams in writable archive async add entry#1310puk06 wants to merge 2 commits into
Conversation
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Pull request overview
Improves async resource cleanup for writable archives so streams opened for newly added/removed/modified entries are released when an archive is disposed via await using, and adds an async tar test intended to catch file-handle leaks after AddEntryAsync(filePath).
Changes:
- Add
DisposeAsyncoverride inAbstractWritableArchive.Async.csto close entries tracked innewEntries,removedEntries, andmodifiedEntries. - Add
TarAsync_AddEntryAndReleaseSourceFiletest to verify the source file can be deleted after async archive creation/disposal.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/SharpCompress/Archives/AbstractWritableArchive.Async.cs | Ensures async disposal closes writable-entry sources tracked outside the base archive entry collection. |
| tests/SharpCompress.Test/Tar/TarArchiveAsyncTests.cs | Adds a regression test aimed at detecting leaked file handles after async tar creation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try | ||
| { | ||
| File.Delete(archivePath); | ||
| File.Delete(sourcePath); | ||
| } | ||
| catch (IOException ex) | ||
| { | ||
| Assert.True(false, "Source file is still locked after disposing archive: " + ex.Message); | ||
| } |
There was a problem hiding this comment.
Catching IOException and then calling Assert.True(false, ...) hides the original stack location of the failure and the message is ambiguous because both archivePath and sourcePath are deleted in the same try block. It’s clearer to let the delete/open failure throw naturally, or split the operations and assert with a message that identifies which file was locked.
| try | |
| { | |
| File.Delete(archivePath); | |
| File.Delete(sourcePath); | |
| } | |
| catch (IOException ex) | |
| { | |
| Assert.True(false, "Source file is still locked after disposing archive: " + ex.Message); | |
| } | |
| File.Delete(archivePath); | |
| File.Delete(sourcePath); |
| new TarWriterOptions(CompressionType.None) { LeaveStreamOpen = false } | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
This test may be flaky under LEGACY_DOTNET (net48) given the existing pattern in this file of adding a post-await using delay because async disposal can lag behind releasing file handles. Consider adding the same #if LEGACY_DOTNET delay after disposing the archive, before attempting to delete the source file.
| #if LEGACY_DOTNET | |
| await Task.Delay(100); | |
| #endif |
| // Assert: after the archive is disposed the source file should no longer be locked | ||
| try | ||
| { | ||
| File.Delete(archivePath); | ||
| File.Delete(sourcePath); | ||
| } | ||
| catch (IOException ex) | ||
| { | ||
| Assert.True(false, "Source file is still locked after disposing archive: " + ex.Message); |
There was a problem hiding this comment.
This test’s assertion relies on Windows-style file locking (delete fails when a file is still open). On Linux/macOS, deleting an in-use file is allowed, so the test can pass even if the source stream is leaked. Consider wrapping the lock/assert portion in #if WINDOWS (or providing a non-Windows variant with a different assertion) so the test meaningfully validates the intended behavior in CI across OSes.
| // Assert: after the archive is disposed the source file should no longer be locked | |
| try | |
| { | |
| File.Delete(archivePath); | |
| File.Delete(sourcePath); | |
| } | |
| catch (IOException ex) | |
| { | |
| Assert.True(false, "Source file is still locked after disposing archive: " + ex.Message); | |
| // Assert: after the archive is disposed the source file should no longer be locked. | |
| // Windows enforces this through delete semantics; Unix-like platforms do not. | |
| if (OperatingSystem.IsWindows()) | |
| { | |
| try | |
| { | |
| File.Delete(archivePath); | |
| File.Delete(sourcePath); | |
| } | |
| catch (IOException ex) | |
| { | |
| Assert.True(false, "Source file is still locked after disposing archive: " + ex.Message); | |
| } | |
| } | |
| else | |
| { | |
| if (File.Exists(archivePath)) | |
| { | |
| File.Delete(archivePath); | |
| } | |
| if (File.Exists(sourcePath)) | |
| { | |
| File.Delete(sourcePath); | |
| } |
| FileAccess.Write, | ||
| FileShare.None, | ||
| bufferSize: 1024 * 1024, | ||
| FileOptions.SequentialScan |
There was a problem hiding this comment.
FileOptions.SequentialScan is primarily a read-optimization hint and is confusing (and potentially ineffective) on a write-only FileStream. Consider removing it or using FileOptions.Asynchronous here if the intent is to optimize async writes.
| FileOptions.SequentialScan | |
| FileOptions.Asynchronous |
|
I've noticed there's a lot of issues around writing and async. I'm working on a fix now |
|
Good luck! |
|
Closing in favor #1311 |
|
I have a question about this previously closed PR. Was await using IWritableAsyncArchive<TarWriterOptions> archive = await TarArchive.CreateAsyncArchive();Could you clarify whether this was intentionally deferred, or if it was perhaps missed in this PR? Thank you. |
|
There is an attempt to do that. That should be fine. Please open a new issue if you think it's wrong I believe I test that here: |
|
Thank you very much. There's a chance it may have been my misunderstanding, so I'll test it on my end as well. |
The existing Tar_Create_New_Async test is not sufficient to reproduce this issue because it only verifies that the archive contents are correct. It does not attempt to delete the output directory immediately after SaveToAsync, so it cannot detect whether the output file is still locked. To reproduce the bug, the test must try to delete the output directory right after SaveToAsync while the writable archive is still in scope. So I’ve opened a new issue, so I’d appreciate it if you could refer to that one. |
This pull request introduces an important resource management improvement to the archive disposal process and adds a new test to ensure source files are properly released after creating a tar archive asynchronously.
Resource management improvements:
DisposeAsyncinAbstractWritableArchive.Async.csto ensure that all entries innewEntries,removedEntries, andmodifiedEntriesare properly closed when the archive is disposed asynchronously. This helps prevent resource leaks and file locks.Testing enhancements:
TarAsync_AddEntryAndReleaseSourceFileinTarArchiveAsyncTests.csto verify that after creating and disposing a tar archive asynchronously, the source file is no longer locked and can be deleted, ensuring correct resource release.