Skip to content

[release/7.0] TarReader should dispose underlying stream if leaveOpen is false - #80598

Merged
carlossanlop merged 3 commits into
dotnet:release/7.0from
jozkee:7.0/tar_dispose
Jan 13, 2023
Merged

[release/7.0] TarReader should dispose underlying stream if leaveOpen is false#80598
carlossanlop merged 3 commits into
dotnet:release/7.0from
jozkee:7.0/tar_dispose

Conversation

@jozkee

@jozkeejozkee commented Jan 13, 2023

Copy link
Copy Markdown
Member

Backport of #79899, #79920, and #80572 to release/7.0

Details

We are not disposing the stream that is wrapped by TarReader even when specifically asked to dispose it with leaveOpen:false.

Customer Impact

This has a customer impact of potentially leaking resources without the user noticing it. as it manifested in our CI, #77012 shows how temporary files were piling up and out-of-disk-space errors happened due to the underlying FileStreams not being properly disposed.

Testing

Unit tests were added to verify that the underlying stream is properly disposed when the TarReader is disposed syncronously and asynchronously.

Risk

Low.

jozkeeand others added 3 commits January 12, 2023 13:56
@jozkee
TarReader should dispose underlying stream if leaveOpen is false (dot…
08e8e19
…net#79899)
@akoeplinger@jozkee
Dispose underlying stream in TarReader.DisposeAsync() as well (dotnet…
1fbde51
…#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
@jozkee
Dispose archive stream after the list of DataStreams (dotnet#80572)
b1f9f65
* Dispose archive stream after the list of DataStreams
* Add tests for TarReader.DisposeAsync properly disposing underlying stream
@jozkeejozkee added the Servicing-consider Issue for next servicing release review label Jan 13, 2023
@jozkeejozkee added this to the 7.0.x milestone Jan 13, 2023
@jozkeejozkee self-assigned this Jan 13, 2023
@ghostghost added the area-System.IO label Jan 13, 2023
@ghost

ghost commented Jan 13, 2023

Copy link
Copy Markdown

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

Issue Details

Backport of #79899, #79920, and #80572 to release/7.0

Details

We are not disposing the stream that is wrapped by TarReader even when specifically asked to dispose it with leaveOpen:false.

Customer Impact

This has a customer impact of potentially leaking resources without the user noticing it. as it manifested in our CI, #77012 shows how temporary files were piling up and out-of-disk-space errors happened due to the underlying FileStreams not being properly disposed.

Testing

Unit tests were added to verify that the underlying stream is properly disposed when the TarReader is disposed syncronously and asynchronously.

Risk

Low.

Author:Jozkee
Assignees:Jozkee
Labels:

Servicing-consider, area-System.IO

Milestone:7.0.x

adamsitnik
adamsitnik approved these changes Jan 13, 2023
View reviewed changes

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@build-analysisbuild-analysisBot mentioned this pull request Jan 13, 2023
Closed
@carlossanlop

carlossanlop commented Jan 13, 2023

Copy link
Copy Markdown
Contributor

CI failures are timeout cancellations in browser (unrelated to this change).

@carlossanlopcarlossanlop modified the milestones: 7.0.x, 7.0.3Jan 13, 2023
@carlossanlopcarlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 13, 2023
@carlossanlop

carlossanlop commented Jan 13, 2023

Copy link
Copy Markdown
Contributor

Approved by Tactics via email (7.0.3).
Signed off by area owners.
No OOB changes needed for this assembly.
CI failures unrelated.
Ready to merge. :shipit:

@carlossanlop
carlossanlop merged commit c8a73af into dotnet:release/7.0Jan 13, 2023
@carlossanlop
carlossanlop deleted the 7.0/tar_dispose branch January 13, 2023 21:01
@ghostghost locked as resolved and limited conversation to collaborators Feb 13, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@adamsitnikadamsitnikadamsitnik approved these changes
+1 more reviewer
@carlossanlopcarlossanlopcarlossanlop approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@jozkeejozkee

Labels

area-System.Formats.TarServicing-approvedApproved for servicing release

Projects

None yet

Milestone

7.0.3

Development

Successfully merging this pull request may close these issues.

4 participants

@jozkee@carlossanlop@adamsitnik@akoeplinger