Skip to content

On shutdown from Python release all slot holders #1720

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

Closed
wants to merge 7 commits into from

Conversation

filmor
Copy link
Member

@filmor filmor commented Mar 10, 2022

What does this implement/fix? Explain your changes.

Before this fix slots of reflected types were not released to Python defaults leaving Python referencing managed code after managed runtime has already shut down.

In the original bug, the runtime in question is Mono after mono_jit_cleanup.

This is an extension of #1712 and actually calls shutdown from Python.

Does this close any currently open issues?

#1683

@filmor
Copy link
Member Author

filmor commented Mar 11, 2022

The errors we are hitting here are the reason why we didn't do the full shutdown in the past. They only occur when running the full suite in sequence (I'm trying to find the minimal permutation that "works", but it's tricky).

@filmor
Copy link
Member Author

filmor commented Mar 27, 2022

The minimal call I have found so far that reproduces the segfault is

pytest --runtime mono tests/test_method.py tests/test_subclass.py

EDIT: Even further:

pytest --runtime mono -k "test_create_instance"

Code for this test:

def test_create_instance():
"""Test derived instances can be created from managed code"""
DerivedClass = derived_class_fixture(test_create_instance.__name__)
ob = FunctionsTest.create_instance(DerivedClass)
assert ob.foo() == "DerivedClass"
assert FunctionsTest.test_foo(ob) == "DerivedClass"
assert ob.bar("bar", 2) == "bar_bar"
assert FunctionsTest.test_bar(ob, "bar", 2) == "bar_bar"
assert ob.not_overriden() == "not_overriden"
x = FunctionsTest.pass_through(ob)
assert id(x) == id(ob)
InterfaceTestClass = interface_test_class_fixture(test_create_instance.__name__)
ob2 = FunctionsTest.create_instance_interface(InterfaceTestClass)
assert ob2.foo() == "InterfaceTestClass"
assert FunctionsTest.test_foo(ob2) == "InterfaceTestClass"
assert ob2.bar("bar", 2) == "bar/bar"
assert FunctionsTest.test_bar(ob2, "bar", 2) == "bar/bar"
y = FunctionsTest.pass_through_interface(ob2)
assert id(y) != id(ob2)

EDIT: Latest result:

If the whole subclass suite is run, there is no crash. If the whole test-suite is run without the subclass, there is no crash either. Pretty :)

Just excluding test_create_instance works reproducibly as well.

EDIT: The exact line creating the leak(?) is

assert ob.not_overriden() == "not_overriden"

@filmor
Copy link
Member Author

filmor commented Mar 29, 2022

@lostmsu This runs through now, but we are not doing the PyObject_GC_Del anymore. Where is the corresponding object creation?

@lostmsu
Copy link
Member

lostmsu commented Mar 29, 2022

@filmor you would be looking for callers of PythonDerivedType.SetPyObj

@filmor filmor force-pushed the bugs/ShutdownSegFault branch from dbb510b to 01f473f Compare March 31, 2022 07:00
@lostmsu
Copy link
Member

lostmsu commented Mar 31, 2022

I know it is annoying, but can you untangle the fix for the issue from the syntax changes?

Also, I think you might have accidentally changed line endings in the interop .tt file.

@lostmsu
Copy link
Member

lostmsu commented Mar 31, 2022

BTW, re the original issue: I haven't looked at it yet, but it might have to do with the default tp_dealloc, tp_clear, or (unlikely) tp_traverse being inadequate for derived type instances with their GC untrack "magic". This situation could probably be detected in the finalizer or during shutdown.

@lostmsu
Copy link
Member

lostmsu commented Apr 8, 2022

Superseded by #1712

@lostmsu lostmsu closed this Apr 8, 2022
@filmor filmor deleted the bugs/ShutdownSegFault branch November 9, 2022 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants