Skip to content

Commit

Permalink
refactor(bmi): BMI adapters' virtual destructor hierarchy and general…
Browse files Browse the repository at this point in the history 
… cleanup (#750)

* cmake: refactor bmi listfile to remove dynamic library call

* bmi: move virtual destructor to Bmi_Adapter, use explicit override on subclasses

* bmi: clean up include directives

* bmi: more headers; cmake: include bmi subdir in search path

* bmi: separate header/impl; fix more includes

* bmi: add missing exception-specifier to AbstractCLibBmiAdapter

* bmi: actually fix noexcept specifier

* fix: add FileChecker include to Bmi_Py_Formulation_Test

* fix: add FileChecker headerr to Bmi_Py_Adapter_Test

* rev: revert move of inline functions to source file

* rev: explicitly show that Bmi_Adapter destructor should be overriden

* rev: revert constness of AbstractCLiBmiAdapter member variable

* bmi: Make Bmi_Adapter uniformly non-moveable

* bmi: Make all adapter classes 'final'

* bmi: Move inclusion of dlopen/dlsym header down to .cpp file

* Remove extraneous semicolon

---------

Co-authored-by: Phil Miller <[email protected]>
  • Loading branch information
@program-- @PhilMiller
program-- and PhilMiller authored Mar 25, 2024
1 parent 3dcc58b commit 9758d43
Show file tree
Hide file tree
Showing 16 changed files with 356 additions and 250 deletions.
120 changes: 8 additions & 112 deletions include/bmi/AbstractCLibBmiAdapter.hpp
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
#ifndef NGEN_ABSTRACTCLIBBMIADAPTER_HPP
#define NGEN_ABSTRACTCLIBBMIADAPTER_HPP

#include <dlfcn.h>
#include "Bmi_Adapter.hpp"
#include "ExternalIntegrationException.hpp"
#include "State_Exception.hpp"

namespace models {
namespace bmi {
Expand All @@ -27,31 +24,15 @@ namespace models {
*/
AbstractCLibBmiAdapter(const std::string &type_name, std::string library_file_path, std::string bmi_init_config,
std::string forcing_file_path, bool allow_exceed_end, bool has_fixed_time_step,
std::string registration_func, utils::StreamHandler output)
: Bmi_Adapter(type_name, std::move(bmi_init_config), std::move(forcing_file_path),
allow_exceed_end, has_fixed_time_step, output),
bmi_lib_file(std::move(library_file_path)),
bmi_registration_function(std::move(registration_func)) { }

AbstractCLibBmiAdapter(AbstractCLibBmiAdapter &&adapter) noexcept :
Bmi_Adapter(std::move(adapter)),
bmi_lib_file(std::move(adapter.bmi_lib_file)),
bmi_registration_function(adapter.bmi_registration_function),
dyn_lib_handle(adapter.dyn_lib_handle)
{
// Have to make sure to do this after "moving" so the original does not close the dynamically loaded library handle
adapter.dyn_lib_handle = nullptr;
}
std::string registration_func, utils::StreamHandler output);

/**
* Class destructor.
*
* Note that this performs the logic in the `Finalize()` function for cleaning up this object and its
* backing BMI model.
*/
virtual ~AbstractCLibBmiAdapter() {
finalizeForLibAbstraction();
}
~AbstractCLibBmiAdapter() override;

/**
* Perform tear-down task for this object and its backing model.
Expand All @@ -67,77 +48,14 @@ namespace models {
*
* @throws models::external::State_Exception Thrown if nested model `finalize()` call is not successful.
*/
void Finalize() override {
finalizeForLibAbstraction();
}
void Finalize() override;

protected:

/**
* Dynamically load and obtain this instance's handle to the shared library.
*/
inline void dynamic_library_load() {
if (bmi_registration_function.empty()) {
this->init_exception_msg =
"Can't init " + this->model_name + "; empty name given for library's registration function.";
throw std::runtime_error(this->init_exception_msg);
}
if (dyn_lib_handle != nullptr) {
this->output.put("WARNING: ignoring attempt to reload dynamic shared library '" + bmi_lib_file +
"' for " + this->model_name);
return;
}
if (!utils::FileChecker::file_is_readable(bmi_lib_file)) {
//Try alternative extension...
size_t idx = bmi_lib_file.rfind(".");
if(idx == std::string::npos){
idx = bmi_lib_file.length()-1;
}
std::string alt_bmi_lib_file;
if(bmi_lib_file.length() == 0){
this->init_exception_msg =
"Can't init " + this->model_name + "; library file path is empty";
throw std::runtime_error(this->init_exception_msg);
}
if(bmi_lib_file.substr(idx) == ".so"){
alt_bmi_lib_file = bmi_lib_file.substr(0,idx) + ".dylib";
} else if(bmi_lib_file.substr(idx) == ".dylib"){
alt_bmi_lib_file = bmi_lib_file.substr(0,idx) + ".so";
} else {
// Try appending instead of replacing...
#ifdef __APPLE__
alt_bmi_lib_file = bmi_lib_file + ".dylib";
#else
#ifdef __GNUC__
alt_bmi_lib_file = bmi_lib_file + ".so";
#endif // __GNUC__
#endif // __APPLE__
}
//TODO: Try looking in e.g. /usr/lib, /usr/local/lib, $LD_LIBRARY_PATH... try pre-pending "lib"...
if (utils::FileChecker::file_is_readable(alt_bmi_lib_file)) {
bmi_lib_file = alt_bmi_lib_file;
} else {
this->init_exception_msg =
"Can't init " + this->model_name + "; unreadable shared library file '" + bmi_lib_file + "'";
throw std::runtime_error(this->init_exception_msg);
}

}

// Call first to ensure any previous error is cleared before trying to load the symbol
dlerror();
// Load up the necessary library dynamically
dyn_lib_handle = dlopen(bmi_lib_file.c_str(), RTLD_NOW | RTLD_LOCAL);
// Now call again to see if there was an error (if there was, this will not be null)
char *err_message = dlerror();
if (dyn_lib_handle == nullptr && err_message != nullptr) {
this->init_exception_msg = "Cannot load shared lib '" + bmi_lib_file + "' for model " + this->model_name;
if (err_message != nullptr) {
this->init_exception_msg += " (" + std::string(err_message) + ")";
}
throw ::external::ExternalIntegrationException(this->init_exception_msg);
}
}
void dynamic_library_load();

/**
* Load and return the address of the given symbol from the loaded dynamic model shared library.
Expand All @@ -161,24 +79,7 @@ namespace models {
* @throws ``std::runtime_error`` If there is not a valid handle already to the shared library.
* @throws ``::external::ExternalIntegrationException`` If symbol could not be found for the shared library.
*/
inline void *dynamic_load_symbol(const std::string &symbol_name, bool is_null_valid) {
if (dyn_lib_handle == nullptr) {
throw std::runtime_error("Cannot load symbol '" + symbol_name + "' without handle to shared library (bmi_lib_file = '" + bmi_lib_file + "')");
}
// Call first to ensure any previous error is cleared before trying to load the symbol
dlerror();
void *symbol = dlsym(dyn_lib_handle, symbol_name.c_str());
// Now call again to see if there was an error (if there was, this will not be null)
char *err_message = dlerror();
if (symbol == nullptr && (err_message != nullptr || !is_null_valid)) {
this->init_exception_msg = "Cannot load shared lib symbol '" + symbol_name + "' for model " + this->model_name;
if (err_message != nullptr) {
this->init_exception_msg += " (" + std::string(err_message) + ")";
}
throw ::external::ExternalIntegrationException(this->init_exception_msg);
}
return symbol;
}
void *dynamic_load_symbol(const std::string &symbol_name, bool is_null_valid);

/**
* Convenience for @see dynamic_load_symbol(std::string, bool) where the symbol address must be found.
Expand All @@ -192,11 +93,11 @@ namespace models {
* @throws ``::external::ExternalIntegrationException`` If symbol could not be found for the shared library.
* @see dynamic_load_symbol(std::string, bool)
*/
inline void *dynamic_load_symbol(const std::string &symbol_name) {
inline void *dynamic_load_symbol(const std::string& symbol_name) {
return dynamic_load_symbol(symbol_name, false);
}

inline const std::string &get_bmi_registration_function() {
inline const std::string& get_bmi_registration_function() {
return bmi_registration_function;
}

Expand All @@ -222,12 +123,7 @@ namespace models {
* Primarily, this exists to contain the functionality appropriate for @see Finalize in a function that is
* non-virtual, and can therefore be called by a destructor.
*/
void finalizeForLibAbstraction() {
// close the dynamically loaded library
if (dyn_lib_handle != nullptr) {
dlclose(dyn_lib_handle);
}
}
void finalizeForLibAbstraction();
};

}
Expand Down
114 changes: 16 additions & 98 deletions include/bmi/Bmi_Adapter.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@

#include <string>
#include <vector>

#include "bmi.hpp"
#include "FileChecker.h"
#include "JSONProperty.hpp"
#include "State_Exception.hpp"
#include "StreamHandler.hpp"
#include <UnitsHelper.hpp>

#include "core/mediator/UnitsHelper.hpp"
#include "utilities/StreamHandler.hpp"


namespace models {
namespace bmi {
Expand All @@ -20,28 +20,12 @@ namespace models {
public:

Bmi_Adapter(std::string model_name, std::string bmi_init_config, std::string forcing_file_path, bool allow_exceed_end,
bool has_fixed_time_step, utils::StreamHandler output)
: model_name(std::move(model_name)),
bmi_init_config(std::move(bmi_init_config)),
bmi_model_uses_forcing_file(!forcing_file_path.empty()),
forcing_file_path(std::move(forcing_file_path)),
bmi_model_has_fixed_time_step(has_fixed_time_step),
allow_model_exceed_end_time(allow_exceed_end),
output(std::move(output)),
bmi_model_time_convert_factor(1.0)
{
// This replicates a lot of Initialize, but it's necessary to be able to do it separately to support
// "initializing" on construction, given using Initialize requires use of virtual functions
errno = 0;
if (!utils::FileChecker::file_is_readable(this->bmi_init_config)) {
init_exception_msg = "Cannot create and initialize " + this->model_name + " using unreadable file '"
+ this->bmi_init_config + "'. Error: "+std::strerror(errno);
throw std::runtime_error(init_exception_msg);
}
}
bool has_fixed_time_step, utils::StreamHandler output);

Bmi_Adapter(Bmi_Adapter const&) = delete;
Bmi_Adapter(Bmi_Adapter &&) = default;
Bmi_Adapter(Bmi_Adapter &&) = delete;

virtual ~Bmi_Adapter() = 0;

/**
* Whether the backing model has been initialized yet.
Expand All @@ -58,12 +42,7 @@ namespace models {
* return an appropriate factor for converting its internal time values to equivalent representations
* within the model, and vice versa. This function complies with the BMI get_time_units standard
*/
double get_time_convert_factor() {
double value = 1.0;
std::string input_units = GetTimeUnits();
std::string output_units = "s";
return UnitsHelper::get_converted_value(input_units, value, output_units);
}
double get_time_convert_factor();

/**
* Convert model time value to value in seconds.
Expand All @@ -74,19 +53,15 @@ namespace models {
* @param model_time_val Arbitrary model time value in units provided by `GetTimeUnits()`
* @return Equivalent model time value to value in seconds.
*/
double convert_model_time_to_seconds(const double& model_time_val) {
return model_time_val * bmi_model_time_convert_factor;
}
double convert_model_time_to_seconds(const double& model_time_val);

/**
* Convert a given number of seconds to equivalent in model time units.
*
* @param seconds_val
* @return
*/
double convert_seconds_to_model_time(const double& seconds_val) {
return seconds_val / bmi_model_time_convert_factor;
}
double convert_seconds_to_model_time(const double& seconds_val);

/**
* Get the name string for the C++ type analogous to the described type in the backing model.
Expand Down Expand Up @@ -130,39 +105,7 @@ namespace models {
* config file could not be read.
* @throws models::external::State_Exception If `initialize()` in nested model does not return successful.
*/
void Initialize() {
// If there was previous init attempt but w/ failure exception, throw runtime error and include previous
// message
errno = 0;
if (model_initialized && !init_exception_msg.empty()) {
throw std::runtime_error(
"Previous " + model_name + " init attempt had exception: \n\t" + init_exception_msg);
}
// If there was previous init attempt w/ (implicitly) no exception on previous attempt, just return
else if (model_initialized) {
return;
}
else if (!utils::FileChecker::file_is_readable(bmi_init_config)) {
init_exception_msg = "Cannot initialize " + model_name + " using unreadable file '"
+ bmi_init_config + "'. Error: "+std::strerror(errno);;
throw std::runtime_error(init_exception_msg);
}
else {
try {
// TODO: make this same name as used with other testing (adjust name in docstring above also)
construct_and_init_backing_model();
// Make sure this is set to 'true' after this function call finishes
model_initialized = true;
bmi_model_time_convert_factor = get_time_convert_factor();
}
// Record the exception message before re-throwing to handle subsequent function calls properly
catch (std::exception& e) {
// Make sure this is set to 'true' after this function call finishes
model_initialized = true;
throw e;
}
}
}
void Initialize();

/**
* Initialize the wrapped BMI model object using the given config file and the object's ``Initialize``
Expand All @@ -182,47 +125,22 @@ namespace models {
* @throws models::external::State_Exception If `initialize()` in nested model is not successful.
* @throws runtime_error If already initialized but using a different file than the passed argument.
*/
void Initialize(std::string config_file) override {
if (config_file != bmi_init_config && model_initialized) {
throw std::runtime_error(
"Model init previously attempted; cannot change config from " + bmi_init_config + " to "
+ config_file);
}

if (config_file != bmi_init_config && !model_initialized) {
output.put("Warning: initialization call changes model config from " + bmi_init_config + " to "
+ config_file);
bmi_init_config = config_file;
}
try {
Initialize();
}
catch (models::external::State_Exception &e) {
throw e;
}
catch (std::exception &e) {
throw std::runtime_error(e.what());
}
}
void Initialize(std::string config_file) override;

/**
* Get whether this instance has been initialized properly.
*
* @return Whether this instance has been initialized properly.
*/
inline bool isInitialized() {
return model_initialized;
}
bool isInitialized();

/**
* @brief Get the model name.
*
* @return The name of the model connected to the adapter.
*
*/
inline std::string get_model_name(){
return model_name;
}
std::string get_model_name();

protected:
/** Whether model ``Update`` calls are allowed and handled in some way by the backing model. */
Expand Down
13 changes: 8 additions & 5 deletions include/bmi/Bmi_C_Adapter.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,13 @@

#include <memory>
#include <string>
#include "AbstractCLibBmiAdapter.hpp"

#include "bmi.h"
#include "JSONProperty.hpp"
#include "StreamHandler.hpp"
#include "AbstractCLibBmiAdapter.hpp"
#include "State_Exception.hpp"
#include "utilities/StreamHandler.hpp"
#include "utilities/ExternalIntegrationException.hpp"


// Forward declaration to provide access to protected items in testing
class Bmi_C_Adapter_Test;
Expand All @@ -20,7 +23,7 @@ namespace models {
* An adapter class to serve as a C++ interface to the essential aspects of external models written in the C
* language that implement the BMI.
*/
class Bmi_C_Adapter : public AbstractCLibBmiAdapter {
class Bmi_C_Adapter final : public AbstractCLibBmiAdapter {

public:

Expand Down Expand Up @@ -101,7 +104,7 @@ namespace models {
*
* Note that this calls the `Finalize()` function for cleaning up this object and its backing BMI model.
*/
virtual ~Bmi_C_Adapter() {
~Bmi_C_Adapter() override {
finalizeForCAdapter();
}

Expand Down
Loading

0 comments on commit 9758d43

Please sign in to comment.