Skip to content

Fixed CollectBasicObject test #1313

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.

Sign up for GitHub

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

Merged
filmor merged 3 commits into from
Dec 12, 2020
Merged

Fixed CollectBasicObject test #1313

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d888244
fixed CollectBasicObject by ensuring MakeAGarbage is not keeping temp…
lostmsu Dec 10, 2020
ff86507
reworked Finalizer to only store raw python object handles instead of…
lostmsu Dec 11, 2020
ca96444
fixed domain reload test, that tries to call static method from objec…
lostmsu Dec 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
fixed CollectBasicObject by ensuring MakeAGarbage is not keeping temp… 
… object alive
  • Loading branch information
@lostmsu
lostmsu committed Dec 11, 2020
commit d88824415637842c8c46ca09355118bd2efd8de2
86 changes: 32 additions & 54 deletions src/embed_tests/TestFinalizer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
using Python.Runtime;
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Diagnostics;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Threading;

namespace Python.EmbeddingTest
Expand All @@ -28,26 +28,14 @@ public void TearDown()
PythonEngine.Shutdown();
}

private static bool FullGCCollect()
private static void FullGCCollect()
{
GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced);
try
{
return GC.WaitForFullGCComplete() == GCNotificationStatus.Succeeded;
}
catch (NotImplementedException)
{
// Some clr runtime didn't implement GC.WaitForFullGCComplete yet.
return false;
}
finally
{
GC.WaitForPendingFinalizers();
}
GC.Collect();
GC.WaitForPendingFinalizers();
}

[Test]
[Ignore("Ignore temporarily")]
[Obsolete("GC tests are not guaranteed")]
public void CollectBasicObject()
{
Assert.IsTrue(Finalizer.Instance.Enable);
Expand Down Expand Up @@ -104,18 +92,13 @@ public void CollectBasicObject()
}

[Test]
[Ignore("Ignore temporarily")]
[Obsolete("GC tests are not guaranteed")]
Copy link
Copy Markdown
Member

@filmor filmor Dec 11, 2020

Choose a reason for hiding this comment

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

Is NUnit aware of this attribute or does this just generate yet another compile-time warning? Does it help to retry the test using https://docs.nunit.org/articles/nunit/writing-tests/attributes/retry.html?

Copy link
Copy Markdown
Member Author

@lostmsu lostmsu Dec 11, 2020

Choose a reason for hiding this comment

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

NUnit is not aware. This actually removes warning from the usage of other [Obsolete] code.

public void CollectOnShutdown()
{
IntPtr op = MakeAGarbage(out var shortWeak, out var longWeak);
int hash = shortWeak.Target.GetHashCode();
List<WeakReference> garbage;
if (!FullGCCollect())
{
Assert.IsTrue(WaitForCollected(op, hash, 10000));
}
FullGCCollect();
Assert.IsFalse(shortWeak.IsAlive);
garbage = Finalizer.Instance.GetCollectedObjects();
List<WeakReference> garbage = Finalizer.Instance.GetCollectedObjects();
Assert.IsNotEmpty(garbage, "The garbage object should be collected");
Assert.IsTrue(garbage.Any(r => ReferenceEquals(r.Target, longWeak.Target)),
"Garbage should contains the collected object");
Expand All @@ -125,12 +108,29 @@ public void CollectOnShutdown()
Assert.IsEmpty(garbage);
}

[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.NoOptimization)] // ensure lack of references to obj
[Obsolete("GC tests are not guaranteed")]
private static IntPtr MakeAGarbage(out WeakReference shortWeak, out WeakReference longWeak)
{
PyLong obj = new PyLong(1024);
shortWeak = new WeakReference(obj);
longWeak = new WeakReference(obj, true);
return obj.Handle;
IntPtr handle = IntPtr.Zero;
WeakReference @short = null, @long = null;
// must create Python object in the thread where we have GIL
IntPtr val = PyLong.FromLong(1024);
// must create temp object in a different thread to ensure it is not present
// when conservatively scanning stack for GC roots.
// see https://xamarin.github.io/bugzilla-archives/17/17593/bug.html
var garbageGen = new Thread(() =>
{
var obj = new PyObject(val, skipCollect: true);
@short = new WeakReference(obj);
@long = new WeakReference(obj, true);
handle = obj.Handle;
});
garbageGen.Start();
Assert.IsTrue(garbageGen.Join(TimeSpan.FromSeconds(5)), "Garbage creation timed out");
shortWeak = @short;
longWeak = @long;
return handle;
}

private static long CompareWithFinalizerOn(PyObject pyCollect, bool enbale)
Expand Down Expand Up @@ -211,6 +211,7 @@ internal static void CreateMyPyObject(IntPtr op)
}

[Test]
[Obsolete("GC tests are not guaranteed")]
public void ErrorHandling()
{
bool called = false;
Expand All @@ -228,7 +229,7 @@ public void ErrorHandling()
WeakReference longWeak;
{
MakeAGarbage(out shortWeak, out longWeak);
var obj = (PyLong)longWeak.Target;
var obj = (PyObject)longWeak.Target;
IntPtr handle = obj.Handle;
shortWeak = null;
longWeak = null;
Expand Down Expand Up @@ -279,36 +280,13 @@ public void ValidateRefCount()
}
}

[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.NoOptimization)] // ensure lack of references to s1 and s2
private static IntPtr CreateStringGarbage()
{
PyString s1 = new PyString("test_string");
// s2 steal a reference from s1
PyString s2 = new PyString(s1.Handle);
return s1.Handle;
}

private static bool WaitForCollected(IntPtr op, int hash, int milliseconds)
{
var stopwatch = Stopwatch.StartNew();
do
{
var garbage = Finalizer.Instance.GetCollectedObjects();
foreach (var item in garbage)
{
// The validation is not 100% precise,
// but it's rare that two conditions satisfied but they're still not the same object.
if (item.Target.GetHashCode() != hash)
{
continue;
}
var obj = (IPyDisposable)item.Target;
if (obj.GetTrackedHandles().Contains(op))
{
return true;
}
}
} while (stopwatch.ElapsedMilliseconds < milliseconds);
return false;
}
}
}
2 changes: 1 addition & 1 deletion src/runtime/pylong.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public PyLong(uint value) : base(FromInt((int)value))
}


private static IntPtr FromLong(long value)
internal static IntPtr FromLong(long value)
{
IntPtr val = Runtime.PyLong_FromLongLong(value);
PythonException.ThrowIfIsNull(val);
Expand Down
13 changes: 13 additions & 0 deletions src/runtime/pyobject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,19 @@ public PyObject(IntPtr ptr)
#endif
}

[Obsolete("for testing purposes only")]
internal PyObject(IntPtr ptr, bool skipCollect)
{
if (ptr == IntPtr.Zero) throw new ArgumentNullException(nameof(ptr));

obj = ptr;
if (!skipCollect)
Finalizer.Instance.ThrottledCollect();
#if TRACE_ALLOC
Traceback = new StackTrace(1);
#endif
}

/// <summary>
/// Creates new <see cref="PyObject"/> pointing to the same object as
/// the <paramref name="reference"/>. Increments refcount, allowing <see cref="PyObject"/>
Expand Down