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 memory error when deallocating Python reaction rate wrappers #1030

Merged
merged 1 commit into from
May 8, 2021

Conversation

speth
Copy link
Member

@speth speth commented May 8, 2021

Using the value of member variables that are Python objects in __dealloc__ is not reliable, because they may have already been
deleted by the time __dealloc__ is called [Cython docs]. Instead, we need a C data member to check for whether or not the rate object owns the underlying C++ object and should delete it.

The only way I've been able to trigger this bug is interactively in IPython, when trying to access members of the rate object with tab autocompletion. I think the reason for this is that this scenario ends up invoking the garbage collector, rather than having objects just get deleted when their reference counts go to zero, and that ends up behaving differently. I'm honestly surprised I didn't run into this before -- this is the way this has worked as long as this information has been accessible in Cython.

Changes proposed in this pull request

  • Add C++ boolean own_rate to Cython Arrhenius and BlowersMasel classes and use this when checking whether to delete the pointer to the corresponding C++ objects.

Checklist

  • There is a clear use-case for this code change
  • The commit message has a short title & references relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

Using the value of member variables that are Python objects in
__dealloc__ is not reliable, because they may have already been
deleted by the time __dealloc__ is called. Instead, we need a C data
member to check for whether or not the rate object owns the underlying
C++ object and should delete it.
Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

Interesting - fix looks good to me.

@ischoegl ischoegl merged commit cfa96aa into Cantera:main May 8, 2021
@speth speth deleted the fix-python-rate-segfault branch May 9, 2021 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants