Fix uninitialized and leaked libRoadrunner instance handle

addons/libRoadrunner/src/librr_intracellular.cpp

@@ -43,7 +43,14 @@ RoadRunnerIntracellular::RoadRunnerIntracellular(RoadRunnerIntracellular* copy)
 	// initial_values = copy->initial_values;
 	// mutations = copy->mutations;
 	parameters = copy->parameters;
-
+	// rrHandle and result are left at their in-class initializers; a RoadRunner instance
+	// cannot be copied, so clone() calls start() to create this one's own.
+}
+
+RoadRunnerIntracellular::~RoadRunnerIntracellular()
+{
+	rrc::freeRRInstance( rrHandle );
+	rrc::freeRRCData( result );
 }
 
 // Parse the <intracellular> info in the .xml for (possibly) each <cell_definition ...>, e.g.
@@ -258,6 +265,18 @@ void RoadRunnerIntracellular::update()
     // return 0;
 }
 
+// Fatal, like the setup-time check in validate_SBML_species(): there is no value to return
+// and nothing sensible to write, so continuing would feed a fabricated number into the model.
+static void librr_unknown_species_fatal(const std::string& species_name, const char* caller)
+{
+    std::cerr << std::endl
+              << "ERROR: " << caller << "() was asked for the SBML species \"" << species_name
+              << "\", which is not present in this model." << std::endl
+              << "       Check the spelling against the species in the SBML file." << std::endl
+              << std::endl;
+    exit(-1);
+}
+
 double RoadRunnerIntracellular::get_parameter_value(std::string param_name)
 {
     rrc::RRVectorPtr vptr;
@@ -277,14 +296,17 @@ double RoadRunnerIntracellular::get_parameter_value(std::string param_name)
     // std::string species_name = this->substrate_species[substrate_name];
     // std::cout << "    species_name = " << species_name << std::endl;
 
-    vptr = rrc::getFloatingSpeciesConcentrations(this->rrHandle);
-    //std::cerr << vptr->Count << std::endl;
-    for (int kdx=0; kdx<vptr->Count; kdx++)
+    // find(), not operator[]: the inserting form silently yields column 0 for an unknown
+    // species, so a typo reads a different species than the caller named, and a read
+    // mutates the map.
+    std::map<std::string,int>::const_iterator it = species_result_column_index.find( param_name );
+    if( it == species_result_column_index.end() )
     {
-        //std::cerr << kdx << ") " << vptr->Data[kdx] << std::endl;
+        librr_unknown_species_fatal( param_name, __FUNCTION__ );
     }
 
-    int offset = species_result_column_index[param_name];
+    vptr = rrc::getFloatingSpeciesConcentrations(this->rrHandle);
+    int offset = it->second;
     //std::cout << "    result offset = "<< offset << std::endl;
     // double res = this->result->Data[offset];
     double res = vptr->Data[offset];
@@ -298,8 +320,16 @@ void RoadRunnerIntracellular::set_parameter_value(std::string species_name, doub
 {
     rrc::RRVectorPtr vptr;
 
+    // see get_parameter_value(): operator[] would silently write column 0 for an unknown
+    // species, i.e. corrupt a different species than the caller named.
+    std::map<std::string,int>::const_iterator it = species_result_column_index.find( species_name );
+    if( it == species_result_column_index.end() )
+    {
+        librr_unknown_species_fatal( species_name, __FUNCTION__ );
+    }
+
     vptr = rrc::getFloatingSpeciesConcentrations(this->rrHandle);
-    int idx = species_result_column_index[species_name];
+    int idx = it->second;
     vptr->Data[idx] = value;
 	// rrc::setFloatingSpeciesConcentrations(pCell->phenotype.molecular.model_rr, vptr);
     rrc::setFloatingSpeciesConcentrations(this->rrHandle, vptr);

addons/libRoadrunner/src/librr_intracellular.h

@@ -47,9 +47,7 @@ class RoadRunnerIntracellular : public PhysiCell::Intracellular
 	std::map<std::string, std::string> phenotype_species;
 	std::map<std::string, int> species_result_column_index;
 
-    // rrc::RRHandle rrHandle = createRRInstance();
-    rrc::RRHandle rrHandle;
-    // rrc::RRHandle rrHandle;
+    rrc::RRHandle rrHandle = nullptr;  // created by start(), released by the destructor
     // rrc::RRVectorPtr vptr;
 	rrc::RRCDataPtr result = 0;  // start time, end time, and number of points
 
@@ -58,9 +56,15 @@ class RoadRunnerIntracellular : public PhysiCell::Intracellular
     RoadRunnerIntracellular();
 
 	RoadRunnerIntracellular(pugi::xml_node& node);
-	
+
 	RoadRunnerIntracellular(RoadRunnerIntracellular* copy);
-
+
+	~RoadRunnerIntracellular();
+
+	// owns rrHandle, so the implicit copies would double-free it; clone() is the way to copy
+	RoadRunnerIntracellular( const RoadRunnerIntracellular& ) = delete;
+	RoadRunnerIntracellular& operator=( const RoadRunnerIntracellular& ) = delete;
+
     // rwh: review this
 	Intracellular* clone()
     {
@@ -70,6 +74,7 @@ class RoadRunnerIntracellular : public PhysiCell::Intracellular
 		clone->substrate_species = this->substrate_species;
         clone->phenotype_species = this->phenotype_species;
 		clone->custom_data_species = this->custom_data_species;
+		clone->start(); // must follow the assignments above: start() loads sbml_filename
 		return static_cast<Intracellular*>(clone);
 	}
 

core/PhysiCell_cell.cpp

@@ -654,7 +654,6 @@ Cell* Cell::divide( )
 	child->phenotype = phenotype; 
 
     if (child->phenotype.intracellular){
-        child->phenotype.intracellular->start();
 		child->phenotype.intracellular->inherit(this);
 	}
 // #ifdef ADDON_PHYSIDFBA
@@ -1124,8 +1123,6 @@ Cell* create_cell( Cell_Definition& cd )
 	pNew->functions = cd.functions; 
 
 	pNew->phenotype = cd.phenotype; 
-	if (pNew->phenotype.intracellular)
-		pNew->phenotype.intracellular->start();
 
 	pNew->is_movable = cd.is_movable; //  true;
 	pNew->is_out_of_domain = false;