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 management errors in ExtensibleRate #1499

Merged
merged 6 commits into from
Jun 11, 2023

Conversation

speth
Copy link
Member

@speth speth commented Jun 10, 2023

Changes proposed in this pull request

  • Don't buffer output from Python when embedding, to avoid confusing ordering of debug log messages
  • Print stack traces after tests that segfault as a debugging aid
  • Update to the attempted fix for Occasional segfault in ExtensibleRate tests #1489 in 2158ff9: we need to use a weak_ptr to know whether the C++ Solution object still exists before calling removeChangedCallback.
  • Fix a reference counting error when creating a PythonHandle from Python

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference 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

speth added 6 commits June 9, 2023 23:09
Fixes a case not handled by 2158ff9 for "weak" wrappers where errors
could occur if the destructor for the weak wrapper was called after the
destructor for the owning Solution object.

Fixes Cantera#1489
The code for instantiating Python Solution wrappers for phases adjacent to an
interface did not link them as the wrapper to be used by ExtensibleRate objects.
@speth speth added the Python label Jun 10, 2023
@codecov
Copy link

codecov bot commented Jun 10, 2023

Codecov Report

Merging #1499 (b1483f1) into main (44c8b6d) will increase coverage by 0.00%.
The diff coverage is 60.00%.

@@           Coverage Diff           @@
##             main    #1499   +/-   ##
=======================================
  Coverage   69.49%   69.50%           
=======================================
  Files         377      377           
  Lines       57995    58011   +16     
  Branches    20693    20695    +2     
=======================================
+ Hits        40305    40318   +13     
- Misses      14739    14741    +2     
- Partials     2951     2952    +1     
Impacted Files Coverage Δ
include/cantera/base/global.h 81.81% <ø> (ø)
src/base/global.cpp 76.92% <33.33%> (-4.99%) ⬇️
interfaces/cython/cantera/_utils.pyx 91.71% <100.00%> (+0.05%) ⬆️
interfaces/cython/cantera/solutionbase.pyx 89.44% <100.00%> (+0.06%) ⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

Thanks, @speth for decoupling this from #1490. Changes all look good to me!

@ischoegl ischoegl merged commit 13cb334 into Cantera:main Jun 11, 2023
@speth speth deleted the fix-extensible-rate branch July 23, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants