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

Fix Python plugin reference leak #776

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dvicini
Copy link
Member

@dvicini dvicini commented Jun 14, 2023

There is a known issue in Mitsuba that a reference to the Python object is leaked when a Python-defined plugin is instantiated.

The original reason we had to leak this reference was to keep the Python object alive beyond the scope of the initialization call in python.h. The problem is that due to this leak, the Python object is never deallocated and may end up in a partially invalid state (e.g., see the nested emitter example in #771).

Ideally, we could fully transfer the ownership of the Python object to C++ and Mitsuba's ref object. This doesn't seem to be possible in pybind11 and as far as I can tell is a known limitation. It will be fixed in nanobind (see the docs).

This PR implements a relatively straightforward fix that should work until the transition to Nanobind is complete. The solution that seems to work is to keep a Python object handle in a cleanup callback lambda, and then explicitly invoke the cleanup call when the object's reference count decreases from 2 to 1 (i.e., the only remaining reference is the Python object). This isn't very elegant but appears to solve the problem on my end.

Not sure if we want to merge this, but I thought I would share this since the transition to Nanobind might still take some time (?). The leaked reference ended up causing some unforeseen issues on my end, so maybe others can benefit from a fix as well.

@njroussel
Copy link
Member

For anyone that comes across this PR, we don't plan on merging these changes. However, we encourage anyone to incorporate this change into their own fork if they are running into issues with leaked references of custom plugins.

As mentioned in the PR description, an upcoming release should properly fix these issues. Until then, I will keep this PR open for visibility.

@wjakob wjakob force-pushed the master branch 3 times, most recently from 3f3b8d0 to 1bdea6e Compare October 12, 2023 19:49
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