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

Refactor Bmi_Adapter and AbstractCLibBmiAdapter to make shared_ptr a unique_ptr #390

Closed
mattw-nws opened this issue Mar 9, 2022 · 1 comment
Labels
bug Something isn't working

Comments

@mattw-nws
Copy link
Contributor

As discovered/discussed in #343, there are some potential issues with handing out a shared_ptr to the underlying bmi_model in the Adapter hierarchy.

Current behavior

bmi_model in Bmi_Adapter is a protected shared_ptr. It doesn't appear at present that there's any way to expose that shared_ptr outside the Bmi_Adapter and its subclasses--this raises the question about whether it should be a shared_ptr. If the pointer is expected to be able to be exposed outside the class, then the lifecycle of dynamically loaded libraries becomes complicated and hard to predict. Particularly, Bmi_Cpp_Adapter needs to call a method from the DLL in the destructor chain to tell the module to destroy the C++ model object. At present this isn't possible because the DLL is closed in AbstractCLibBmiAdapter and bmi_model is destroyed it its parent class Bmi_Adapter--so the sequence is wrong--therefore the module cleanup function is called in Finalize instead. But, if the bmi_model is exposed and stored outside the Bmi_Adapter hierarchy, then the opposite could occur: the Bmi_Adapter could be disposed, closing the DLL but leaving the bmi_model reference in the shared_ptr undisposed but with the destroy function already called in the DLL--which is supposed to free the object pointed to by bmi_model (and even if it didn't, any use of it would fail because the DLL handle would have been closed).

Expected behavior

bmi_model should probably be a unique_ptr instead, unless there's a need to expose it outside the class hierarchy. If so, then the DLL loading lifecycle may need to be refactored to be more predictable.

@mattw-nws mattw-nws added the bug Something isn't working label Mar 9, 2022
@hellkite500
Copy link
Contributor

Addressed in #646

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants