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

Disable frequently failing CriticalFinalizer test #76131

Merged
merged 2 commits into from
Sep 25, 2022
Merged

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Sep 24, 2022

Contributes to #76041

@ghost ghost assigned jkotas Sep 24, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@filipnavara
Copy link
Member

I thought https://github.com/dotnet/runtime/pull/76089/files was already supposed to do that and got merged...

@jkotas
Copy link
Member Author

jkotas commented Sep 24, 2022

I thought https://github.com/dotnet/runtime/pull/76089/files was already supposed to do that and got merged...

It disabled the test for coreclr only. The test is failing everywhere else as well. And the issue was not labelled disabled-test so I did not even know that this was done.

@VSadov
Copy link
Member

VSadov commented Sep 24, 2022

It looks like #76089 did this incorrectly. The format of this file is confusing. - there should be a name of the test (not the containing folder) followed by a "/".

Also I disabled the test for coreclr only, but it fails on Mono as well, with the same "Finalized 0 Normal and 0 Critical objects." failure.
That likely indicates the issue with the test. Hard to imagine the same bug in two different GCs.

@jkotas
Copy link
Member Author

jkotas commented Sep 24, 2022

That likely indicates the issue with the test.

Yes, it looks like a test issue.

Note that this test was recently moved from old Mono test bed that was not running to the actively running tests

@VSadov
Copy link
Member

VSadov commented Sep 24, 2022

That likely indicates the issue with the test. Hard to imagine the same bug in two different GCs.

There is still a possibility that there are different reasons for objects not collected - Mono does not guarantee anything since the stack scan is conservative, although the stack should be nearly empty when Collect() is called in the test, so chances for that being the reason are very low. The test originates from mono, so it was passing for a very long time (if it was running).

The reason for CoreCLR failing to collect is harder to guess. Collect(); WaitForPendingFinalizers(); should be deterministic.

Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

Thanks!

@VSadov
Copy link
Member

VSadov commented Sep 24, 2022

moved from old Mono test bed that was not running

Not running? That would explain failures on Mono.

@AntonLapounov
Copy link
Member

For CoreCLR, I have found that either setting arr = null or doing GC.Collect(); GC.WaitForPendingFinalizers(); two times in a row fixes the issue, which may indicate the arr array remains rooted on the first GC.Collect() call. Apparently, a number of existing tests for finalizers also call GC.Collect() twice in a row, e.g.,

GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();

Also, it is much easier to reproduce the issue with tiered compilation turned off.

The original Mono test would not use the array of references and it would pass if at least one of the Normal (P) instances out of 100 was collected, so it used to be much weaker (and not compatible with GC stress).

Copy link
Member

@AntonLapounov AntonLapounov left a comment

Choose a reason for hiding this comment

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

Thanks!

@AntonLapounov
Copy link
Member

The failure in 'CoreCLR Product Build windows x86 release' is #48070.

@VSadov
Copy link
Member

VSadov commented Sep 25, 2022

I suppose, since this is blocking clean CI and tests are passing, we can merge.

@VSadov VSadov merged commit c986a12 into dotnet:main Sep 25, 2022
@jkotas jkotas deleted the 76041 branch September 25, 2022 02:07
@ghost ghost locked as resolved and limited conversation to collaborators Oct 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants