Skip to content
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

Merged
merged 3 commits into from
Dec 23, 2022

Conversation

akoeplinger
Copy link
Member

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 on WrappedStream not disposing the stream internally so I just consolidated to a single source file and and separately dispose of the stream passed to WrappedStream in the System.Formats.Tar tests.

@ghost
Copy link

ghost commented Dec 23, 2022

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

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 on WrappedStream not disposing the stream internally so I just consolidated to a single source file and and separately dispose of the stream passed to WrappedStream in the System.Formats.Tar tests.

Author: akoeplinger
Assignees: akoeplinger
Labels:

area-System.IO

Milestone: -

@build-analysis build-analysis bot mentioned this pull request Dec 23, 2022
using System.IO;

namespace System.Formats.Tar
namespace System.IO
{
public class WrappedStream : Stream
Copy link
Member Author

@akoeplinger akoeplinger Dec 23, 2022

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.

Copy link
Member

@adamsitnik adamsitnik left a 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 !

@adamsitnik
Copy link
Member

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@adamsitnik
Copy link
Member

/azp run runtime-libraries-coreclr outerloop-linux

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
Copy link
Member

Some of the outerloops tests have failed due to timeouts:

  Discovering: System.Formats.Tar.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Formats.Tar.Tests (found 2 of 635 test cases)
  Starting:    System.Formats.Tar.Tests (parallel test collections = on, max threads = 2)
   System.Formats.Tar.Tests: [Long Running Test] 'System.Formats.Tar.Tests.TarWriter_WriteEntry_LongFile_Tests.WriteEntry_LongFileSize', Elapsed: 00:03:59
   System.Formats.Tar.Tests: [Long Running Test] 'System.Formats.Tar.Tests.TarWriter_WriteEntry_LongFile_Tests.WriteEntry_LongFileSize', Elapsed: 00:06:00
   System.Formats.Tar.Tests: [Long Running Test] 'System.Formats.Tar.Tests.TarWriter_WriteEntry_LongFile_Tests.WriteEntry_LongFileSize', Elapsed: 00:08:00
   System.Formats.Tar.Tests: [Long Running Test] 'System.Formats.Tar.Tests.TarWriter_WriteEntry_LongFile_Tests.WriteEntry_LongFileSize', Elapsed: 00:10:00
   System.Formats.Tar.Tests: [Long Running Test] 'System.Formats.Tar.Tests.TarWriter_WriteEntryAsync_LongFile_Tests.WriteEntry_LongFileSizeAsync', Elapsed: 00:02:08
   System.Formats.Tar.Tests: [Long Running Test] 'System.Formats.Tar.Tests.TarWriter_WriteEntryAsync_LongFile_Tests.WriteEntry_LongFileSizeAsync', Elapsed: 00:04:08
['System.Formats.Tar.Tests' END OF WORK ITEM LOG: Command timed out, and was killed]

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.

@adamsitnik adamsitnik merged commit 372adbf into dotnet:main Dec 23, 2022
@akoeplinger akoeplinger deleted the tarreader branch December 23, 2022 16:59
jozkee pushed a commit to jozkee/runtime that referenced this pull request Jan 13, 2023
…#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
carlossanlop pushed a commit that referenced this pull request Jan 13, 2023
… 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 <alex.koeplinger@outlook.com>
@ghost ghost locked as resolved and limited conversation to collaborators Jan 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants