-
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
Clean up tar file at the end of WriteEntry_LongFileSizeAsync test #79907
Conversation
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(); |
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.
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.
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.
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)
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.
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.
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.
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.
seems to be specific to the Async test
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.
ahhh the fix in #79899 didn't get applied to the DisposeAsync()
method 😆
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.
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.
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.
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.
see #79920
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.