-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Dispose underlying stream in TarReader.DisposeAsync() as well #79920
Conversation
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsSame as #79899 but for the async dispose method. In the System.Formats.Tar WriteEntry_LongFileSize/WriteEntry_LongFileSizeAsync tests the stream passed to
|
using System.IO; | ||
|
||
namespace System.Formats.Tar | ||
namespace System.IO | ||
{ | ||
public class WrappedStream : Stream |
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.
@carlossanlop do you remember which changes you meant back in #67883 (comment) ?
It seems to work with the consolidated version.
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.
LGTM, big thanks for your help and the fix @akoeplinger !
/azp list |
/azp run runtime-libraries-coreclr outerloop-linux |
Azure Pipelines successfully started running 1 pipeline(s). |
Some of the outerloops tests have failed due to timeouts:
I am going to merge this PR as it's fixing a product bug and reopen #77012 to keep track of the required test changes. |
…#79920) * Dispose underlying stream in TarReader.DisposeAsync() as well Same as dotnet#79899 * Consolidate duplicated WrappedStream test helpers to Common sources * Dispose stream passed to WrappedStream
… is false (#80598) * TarReader should dispose underlying stream if leaveOpen is false (#79899) * Dispose underlying stream in TarReader.DisposeAsync() as well (#79920) * Dispose underlying stream in TarReader.DisposeAsync() as well Same as #79899 * Consolidate duplicated WrappedStream test helpers to Common sources * Dispose stream passed to WrappedStream * Dispose archive stream after the list of DataStreams (#80572) * Dispose archive stream after the list of DataStreams * Add tests for TarReader.DisposeAsync properly disposing underlying stream Co-authored-by: Alexander Köplinger <[email protected]>
Same as #79899 but for the async dispose method.
In the System.Formats.Tar WriteEntry_LongFileSize/WriteEntry_LongFileSizeAsync tests the stream passed to
WrappedStream
was also not disposed, causing it to stick around longer (see #79907). While looking at adding that I found out we have two nearly identical copies of the helper class in both System.Formats.Tar and System.IO.Compression, but a test in the latter one relies onWrappedStream
not disposing the stream internally so I just consolidated to a single source file and and separately dispose of the stream passed toWrappedStream
in the System.Formats.Tar tests.