Access violation dealing with Wall between empty Reactors

[Naikless]

Problem description Trying to access heat_rate (and probably other stuff too) from a Wall connecting two empty Reactors (i.e. with no defined phase) crashes the interpreter.

Clearly, there should be no proper result without defined phases in the reactors, but an access violation seems a bit drastic to me.

Steps to reproduce

import cantera as ct

r1 = ct.Reactor()
r2 = ct.Reactor()

w = ct.Wall(r1,r2)

w.heat_rate

Behavior The result for me is

Windows fatal exception: access violation

I suppose there is just a check missing on the C++ side of the object, because the conceptually similar MassFlowController handles the same situation by throwing

CanteraError: 
*******************************************************************************
CanteraError thrown by ReactorBase::contents:
Reactor contents not defined.
*******************************************************************************

I suppose copying this behavior to the Wall object would be enough.

System information

• Cantera version: 3.1.0a1
• OS: Windows 10
• Python/MATLAB/other software versions: Python 3.11.5

[ischoegl]

@Naikless thank you for reporting. I can reproduce this on macOS. Indeed, there appears to be a missing guard.

[speth]

Is being able to create an empty Reactor a useful feature? I wonder if the right place to enforce this is there. Doing that might save a lot of easy-to-forget checks like this one.

[ischoegl]

Is being able to create an empty Reactor a useful feature? I wonder if the right place to enforce this is there. Doing that might save a lot of easy-to-forget checks like this one.

@speth ... I believe you're right about this. The only scenario where an empty reactor may be useful is a "reservoir" along the lines of something that I wrote up a long time ago, see https://github.com/Cantera/enhancements/discussions/92. This is a long way from being implemented (if it ever is), so I'd be :+1: with simply throwing an error for an empty Reactor.

There still is the left-over issue with removing raw pointers mentioned in https://github.com/Cantera/cantera/issues/1457, which could be addressed at the same time. Edit: emphasis on "could", as that fix is definitely a lot more involved.

[speth]

Yes, agreed on the caveat -- I think a lighter-weight Reservoir without a ThermoPhase would be a useful feature, and I wouldn't want to preculde that. So, this would correspond to requiring a non-empty Reactor, but allowing an empty ReactorBase.

[Naikless]

Just to add context, I stumbled upon this by toying around with reactor networks and testing my visualization enhancement.

If this makes it into the code, there is a scenario conceivable where someone just wants to "stich together" a few reactors to be able to visualize their network concept.

In this case, empty reactors might be useful. But I don't know if this edge case is worth the extra maintenance.

[ischoegl]

@Naikless ... I don't think that the scenario of drafting something warrants an empty Reactor as we are more concerned with the creation of reactor networks that can be simulated. Also, it is very easy to assign a ThermoPhase object if one really only wanted to visualize.

@speth ... my bad, but I just realized that Reservoir is an existing class (which I myself rarely use).

[Naikless]

Just to add: Similarly violent terminations also occur for ReactorSurface objects, when created without either fully defined reactor or interface kinetics.

I suppose disallowing these "empty" objects (at least for the non-abstract classes) by making the corresponding phase objects mandatory arguments would be the most straightforward solution then.

[ischoegl]

Just to add: Similarly violent terminations also occur for ReactorSurface objects, when created without either fully defined reactor or interface kinetics.

I suppose disallowing these "empty" objects (at least for the non-abstract classes) by making the corresponding phase objects mandatory arguments would be the most straightforward solution then.

Thanks for confirming. No surprises …