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

Clean up tar file at the end of WriteEntry_LongFileSizeAsync test #79907

Closed
wants to merge 1 commit into from

Conversation

akoeplinger
Copy link
Member

Otherwise multiple tar files could stick around for some time, contributing to issues like #77012

With this I've never seen disk usage go below the 8GB that a single test run takes.

Otherwise multiple tar files could stick around for some time, contributing to issues like dotnet#77012
@@ -86,6 +86,7 @@ public void WriteEntry_LongFileSize(TarEntryFormat entryFormat, long size, bool
Assert.Equal(size, dataStream.Position);

Assert.Null(reader.GetNextEntry());
tarFile.Close();
Copy link
Member

Choose a reason for hiding this comment

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

I would expect #79899 to solve this problem for good. If that is not the case, we need to investigate it and ensure that Dispose does the right thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

it didn't solve the issue for me, at least on Mono. I haven't checked on CoreCLR (GC behavior can be quite different between the runtimes)

Copy link
Member Author

Choose a reason for hiding this comment

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

ah I see, I missed that there's a using when creating the TarReader so we should expect it getting disposed at the end of the test.

Copy link
Member Author

@akoeplinger akoeplinger Dec 22, 2022

Choose a reason for hiding this comment

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

hmm, interesting: without the change in this PR it looks like the file does get deleted on disk however the process still keeps it open at least according to Activity Monitor on Mac:

image

and it also still counts against the Available disk space in df -h.

Copy link
Member Author

Choose a reason for hiding this comment

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

seems to be specific to the Async test

Copy link
Member Author

Choose a reason for hiding this comment

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

ahhh the fix in #79899 didn't get applied to the DisposeAsync() method 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

ok and another problem is that the WrappedStream isn't disposing the inner _baseStream. I'll be away for a few hours now, but I can send a PR later.

Copy link
Member

Choose a reason for hiding this comment

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

@akoeplinger thank you for investigating it!

I can send a PR later.

That would be perfect.

Copy link
Member Author

Choose a reason for hiding this comment

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

see #79920

@akoeplinger akoeplinger added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 22, 2022
@build-analysis build-analysis bot mentioned this pull request Dec 22, 2022
@akoeplinger akoeplinger deleted the tarfile-close branch December 23, 2022 00:39
@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.
Labels
area-System.Formats.Tar NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants