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

addSpecies should be disabled on ThermoPhase objects in use by reactors and flames #1457

Closed
speth opened this issue Mar 18, 2023 · 1 comment · Fixed by #1686
Closed

addSpecies should be disabled on ThermoPhase objects in use by reactors and flames #1457

speth opened this issue Mar 18, 2023 · 1 comment · Fixed by #1686
Labels

Comments

@speth
Copy link
Member

speth commented Mar 18, 2023

Problem description

The ThermoPhase::addSpecies method can be used to add species to a phase dynamically. However, this capability is not compatible with objects such as Reactor or Domain1D, which set internal array sizes based on the number of species in the phase at the time they are created.

In Python, this is protected against by having Solution objects hold a weak reference any Reactor, Domain1D, or Mixture objects that use that Solution. No equivalent exists at the C++ layer, though, and adding species after creating such objects can lead to crashes.

Managing this in a similar way in C++ would have been difficult until recently, but I think the transition to having objects like Domain1D hold the Solution as a shared_ptr (see #1385, for example) should make this more feasible after some of the old routes have been removed following Cantera 3.0. I think one prerequisite will be using shared_ptr<Solution> to set up Reactor objects, which have not had this change yet.

Steps to reproduce

Replace the integration loop in samples/cxx/combustor/combustor.cpp with the following:

    bool added = false;
    while (tnow < tfinal) {
        tnow += 0.005;
        sim.advance(tnow);
        tres = combustor.mass()/v.massFlowRate();
        f << tnow << ", "
          << combustor.temperature() << ", "
          << tres << ", ";
        ThermoPhase& c = combustor.contents();
        for (size_t i = 0; i < k_out.size(); i++) {
            f << c.moleFraction(k_out[i]) << ", ";
        }
        f << std::endl;
        if (!added) {
            auto c6h6 = make_shared<Species>("C6H6", Composition({{"C", 6}, {"H", 6}}));
            auto spthermo = c6h6->thermo->parameters();
            spthermo.applyUnits();
            c6h6->thermo = newSpeciesThermo(spthermo);
            gas->addSpecies(c6h6);
            added = true;
        }
    }

(also requires adding includes for cantera/thermo/Species.h and cantera/thermo/SpeciesThermoFactory.h)

Behavior

The above segfaults after the first timestep.

Additional context

Initially discussed in PR review for #1456.

@ischoegl
Copy link
Member

ischoegl commented Mar 30, 2024

#1663 and #1670/#1685 ensure that individual ThermoPhase and Kinetics objects are always tied to a Solution object. With this, the _WeakrefProxy guards existing in Python can be moved to C++, which should allow for a clean resolution?

PS: there still is an issue with MultiPhase that requires resolution.

PPS: the relevant section describing this is

# In composite objects, the ThermoPhase constructor needs to be called first
# to prevent instantiation of stand-alone 'Kinetics' or 'Transport' objects.
# The following is used as a sentinel. After initialization, the _references
# object is used to track whether the ThermoPhase is being used by a `Reactor`,
# `Domain1D`, or `Mixture` object and to prevent species from being added to the
# ThermoPhase if so, since these objects require the number of species to remain
# constant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants