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

WeakReference behaves differently from .NET Framework. #12847

Closed
ngbrown opened this Issue Jul 17, 2017 · 6 comments

Comments

Projects
None yet
4 participants
@ngbrown

ngbrown commented Jul 17, 2017

I am porting NHibernate to .NET Core (nhibernate/nhibernate-core#633), and some of the tests related to WeakReference are not passing.

The same tests have been running on .NET Framework.

Please see the example test here: https://github.com/ngbrown/coreclr-WeakReferenceTest

Running:

dotnet test -f net461

passes, while running:

dotnet test -f netcoreapp2.0

fails with:

NUnit3TestExecutor converted 7 of 7 NUnit test cases
NUnit Adapter 3.8.0.0: Test execution complete
Failed   IterationAfterGC
Error Message:
   should not have live elements
  Expected: False
  But was:  True

Stack Trace:
at NHibernate.Test.UtilityTest.WeakHashtableFixture.IterationAfterGC() in C:\dev\test\WeakReferenceTest\WeakHashtableFixture.cs:line 71

Failed   Scavenging
Error Message:
   Expected: 0
  But was:  2

Stack Trace:
at NHibernate.Test.UtilityTest.WeakHashtableFixture.Scavenging() in C:\dev\test\WeakReferenceTest\WeakHashtableFixture.cs:line 57

Failed   WeakReferenceGetsFreedButHashCodeRemainsConstant
Error Message:
   Expected: False
  But was:  True

Stack Trace:
at NHibernate.Test.UtilityTest.WeakHashtableFixture.WeakReferenceGetsFreedButHashCodeRemainsConstant() in C:\dev\test\WeakReferenceTest\WeakHashtableFixture.cs:line 41


Total tests: 7. Passed: 4. Failed: 3. Skipped: 0.
Test Run Failed.
Test execution time: 2.0657 Seconds

I think the summary is that weak references don't appear to actually be getting freed like they should be during garbage collection. This means we can't be sure that our query cache (which WeakHashtable is used for) won't be a memory leak problem.

Version info:

> dotnet --info
.NET Command Line Tools (2.0.0-preview2-006497)

Product Information:
 Version:            2.0.0-preview2-006497
 Commit SHA-1 hash:  06a2093335

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.15063
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\2.0.0-preview2-006497\

Microsoft .NET Core Shared Framework Host

  Version  : 2.0.0-preview2-25407-01
  Build    : 40c565230930ead58a50719c0ec799df77bddee9
@ngbrown

This comment has been minimized.

ngbrown commented Jul 17, 2017

I wasn't 100% sure where to put this issue, because coreclr implements WeakReference and corefx has some tests for it.

From those tests, they sometimes did these three calls before checking stuff:

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

but changing to that from a single GC.Collect(); didn't change the results of our tests.

@jkotas

This comment has been minimized.

Member

jkotas commented Jul 17, 2017

The JIT has been always extending the lifetime of the local and temp variables in some cases - the spec allows that. It started doing it in more cases after change #9231.

We had tests that used to depend on the exact lifetime tracking as well that needed updating after this change, e.g. dotnet/corefx#16595 .

@jkotas jkotas added the area-CodeGen label Jul 17, 2017

@ngbrown

This comment has been minimized.

ngbrown commented Jul 17, 2017

Does WeakReference also depend on where in the stack it was created at or GC.Collect() was run at? I don't really understand why MethodImplOptions.NoInlining is so important to the changes that happened.

Any suggestions for the test code?

@stephentoub

This comment has been minimized.

Member

stephentoub commented Jul 17, 2017

Any suggestions for the test code?

Put the:

table[new object()] = new object();
table[new object()] = new object();

lines into their own non-inlined method so that the JIT doesn't have the ability to extend beyond the lifetime of the method any temporaries keeping those objects alive.

@swgillespie

This comment has been minimized.

Contributor

swgillespie commented Jul 17, 2017

@ngbrown Here's a PR where I fixed some of CoreCLR's own tests that were running into trouble with the Collect/WaitForPendingFinalizers/Collect pattern: #11216. Unfortunately these sorts of tests are racy in general due to the asynchrony of finalization but the fixes I made in that PR seem to have fixed the tests (in addition to @stephentoub 's suggestion)

@ngbrown

This comment has been minimized.

ngbrown commented Jul 18, 2017

Thanks everyone. Putting the temporary objects into a non-inlined function fixed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment