Skip to content

Bug/220 spice makes bsk sims not thread safe #994

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Bug/220 spice makes bsk sims not thread safe #994

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d3d7467
Mutex for thread safety and ref counting
Natsoulas May 7, 2025
7389555
Add new test for spice thread safety
Natsoulas May 7, 2025
a9ef12b
Update Release Notes
Natsoulas May 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 119 additions & 41 deletions src/simulation/environment/spiceInterface/spiceInterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@
#include "architecture/utilities/macroDefinitions.h"
#include "architecture/utilities/rigidBodyKinematics.h"

// Initialize static members
std::mutex SpiceInterface::kernel_manipulation_mutex;
std::map<std::string, int> SpiceInterface::kernel_reference_counter;

/*! This constructor initializes the variables that spice uses. Most of them are
not intended to be changed, but a couple are user configurable.
*/
Expand Down Expand Up @@ -74,15 +78,34 @@ SpiceInterface::~SpiceInterface()
delete this->transRefStateOutMsgs.at(c);
}
delete [] this->spiceBuffer;
// if(this->SPICELoaded)
// {
// this->clearKeeper();
// }

// Properly unload kernels if they were loaded
if(this->SPICELoaded)
{
// Create vector instead of variable-length array for Windows compatibility
std::vector<char> kernelNameVec(this->charBufferSize);
char* kernelName = kernelNameVec.data();

// Unload the SPICE kernels in reverse order of loading
strcpy(kernelName, "de430.bsp");
unloadSpiceKernel(kernelName, this->SPICEDataPath.c_str());

strcpy(kernelName, "de-403-masses.tpc");
unloadSpiceKernel(kernelName, this->SPICEDataPath.c_str());

strcpy(kernelName, "pck00010.tpc");
unloadSpiceKernel(kernelName, this->SPICEDataPath.c_str());

strcpy(kernelName, "naif0012.tls");
unloadSpiceKernel(kernelName, this->SPICEDataPath.c_str());
Comment on lines +85 to +100
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you change the signature of unloadSpiceKernel (see previous comment), you'll be able to call:

unloadSpiceKernel("de430.bsp", this->SPICEDataPath);

}

return;
}

void SpiceInterface::clearKeeper()
{
std::lock_guard<std::mutex> lock(kernel_manipulation_mutex);
kclear_c();
}

Expand All @@ -101,18 +124,32 @@ void SpiceInterface::Reset(uint64_t CurrenSimNanos)
//!- Load the SPICE kernels if they haven't already been loaded
if(!this->SPICELoaded)
{
if(loadSpiceKernel((char *)"naif0012.tls", this->SPICEDataPath.c_str())) {
bskLogger.bskLog(BSK_ERROR, "Unable to load %s", "naif0012.tls");
// Create vector instead of variable-length array for Windows compatibility
std::vector<char> kernelNameVec(this->charBufferSize);
char* kernelName = kernelNameVec.data();

// Load the required SPICE kernels - they will only be loaded once per kernel
// across all threads due to our reference counting mechanism
strcpy(kernelName, "naif0012.tls");
if(loadSpiceKernel(kernelName, this->SPICEDataPath.c_str())) {
bskLogger.bskLog(BSK_ERROR, "Unable to load %s", kernelName);
}
if(loadSpiceKernel((char *)"pck00010.tpc", this->SPICEDataPath.c_str())) {
bskLogger.bskLog(BSK_ERROR, "Unable to load %s", "pck00010.tpc");

strcpy(kernelName, "pck00010.tpc");
if(loadSpiceKernel(kernelName, this->SPICEDataPath.c_str())) {
bskLogger.bskLog(BSK_ERROR, "Unable to load %s", kernelName);
}
if(loadSpiceKernel((char *)"de-403-masses.tpc", this->SPICEDataPath.c_str())) {
bskLogger.bskLog(BSK_ERROR, "Unable to load %s", "de-403-masses.tpc");

strcpy(kernelName, "de-403-masses.tpc");
if(loadSpiceKernel(kernelName, this->SPICEDataPath.c_str())) {
bskLogger.bskLog(BSK_ERROR, "Unable to load %s", kernelName);
}
if(loadSpiceKernel((char *)"de430.bsp", this->SPICEDataPath.c_str())) {
bskLogger.bskLog(BSK_ERROR, "Unable to load %s", "de430.tpc");

Comment on lines +128 to +147
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might want to refactor this into:

auto kernelNames = {
    "naif0012.tls", "pck00010.tpc", "de-403-masses.tpc", "de430.bsp"
};
for (auto kernelName : kernelNames)
{
    if(loadSpiceKernel(kernelName, this->SPICEDataPath)) {
        bskLogger.bskLog(BSK_ERROR, "Unable to load %s", kernelName);
    }
}

actually a similar refactor is also possible above for unloadSpiceKernels. You might even make the kernelNames a class-wide static variable so we don't need to update both loading and unloading methods to keep the kernels in sync.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, this is a cleaner solution.

strcpy(kernelName, "de430.bsp");
if(loadSpiceKernel(kernelName, this->SPICEDataPath.c_str())) {
bskLogger.bskLog(BSK_ERROR, "Unable to load %s", kernelName);
}

this->SPICELoaded = true;
}

Expand Down Expand Up @@ -436,25 +473,44 @@ void SpiceInterface::pullSpiceData(std::vector<SpicePlanetStateMsgPayload> *spic
*/
int SpiceInterface::loadSpiceKernel(char *kernelName, const char *dataPath)
{
char *fileName = new char[this->charBufferSize];
SpiceChar *name = new SpiceChar[this->charBufferSize];
// Create vectors instead of raw pointers for Windows compatibility
std::vector<char> fileNameVec(this->charBufferSize);
char* fileName = fileNameVec.data();

std::vector<char> nameVec(this->charBufferSize);
char* name = nameVec.data();

//! - The required calls come from the SPICE documentation.
//! - The most critical call is furnsh_c
strcpy(name, "REPORT");
erract_c("SET", this->charBufferSize, name);
// Create the full filepath
strcpy(fileName, dataPath);
strcat(fileName, kernelName);
furnsh_c(fileName);

//! - Check to see if we had trouble loading a kernel and alert user if so
strcpy(name, "DEFAULT");
erract_c("SET", this->charBufferSize, name);
delete[] fileName;
delete[] name;
if(failed_c()) {
return 1;
std::string filepath(fileName);
Comment on lines +476 to +486
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you refactor the inputs into std::string_view, then:

std::string filepath = std::string{dataPath} + std::string{kernelName);


// Acquire the mutex to protect kernel operations
std::lock_guard<std::mutex> lock(kernel_manipulation_mutex);

// Initialize the reference counter for this kernel if it doesn't exist
kernel_reference_counter.try_emplace(filepath, 0);

// Only load the kernel if it hasn't been loaded yet
if (kernel_reference_counter.at(filepath) <= 0) {
// The required calls come from the SPICE documentation.
// The most critical call is furnsh_c
strcpy(name, "REPORT");
erract_c("SET", this->charBufferSize, name);
furnsh_c(fileName);

// Check to see if we had trouble loading a kernel
strcpy(name, "DEFAULT");
erract_c("SET", this->charBufferSize, name);

if(failed_c()) {
return 1;
}
Comment on lines +495 to +508
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job fetching the error from SPICE.

}

// Increment the reference counter for this kernel
kernel_reference_counter[filepath]++;

return 0;
}

Expand All @@ -468,27 +524,47 @@ int SpiceInterface::loadSpiceKernel(char *kernelName, const char *dataPath)
*/
int SpiceInterface::unloadSpiceKernel(char *kernelName, const char *dataPath)
{
char *fileName = new char[this->charBufferSize];
SpiceChar *name = new SpiceChar[this->charBufferSize];
// Create vectors instead of raw pointers for Windows compatibility
std::vector<char> fileNameVec(this->charBufferSize);
char* fileName = fileNameVec.data();

//! - The required calls come from the SPICE documentation.
//! - The most critical call is furnsh_c
strcpy(name, "REPORT");
erract_c("SET", this->charBufferSize, name);
std::vector<char> nameVec(this->charBufferSize);
char* name = nameVec.data();

// Create the full filepath
strcpy(fileName, dataPath);
strcat(fileName, kernelName);
unload_c(fileName);
delete[] fileName;
delete[] name;
if(failed_c()) {
return 1;
std::string filepath(fileName);
Comment on lines +528 to +537
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment as above.


// Acquire the mutex to protect kernel operations
std::lock_guard<std::mutex> lock(kernel_manipulation_mutex);

// Initialize the reference counter for this kernel if it doesn't exist
kernel_reference_counter.try_emplace(filepath, 0);

// Decrement the reference counter
kernel_reference_counter[filepath]--;

// Only unload if no more references to this kernel
if (kernel_reference_counter.at(filepath) <= 0) {
// The required calls come from the SPICE documentation.
strcpy(name, "REPORT");
erract_c("SET", this->charBufferSize, name);
unload_c(fileName);

strcpy(name, "DEFAULT");
erract_c("SET", this->charBufferSize, name);

if(failed_c()) {
return 1;
}
}

return 0;
}

std::string SpiceInterface::getCurrentTimeString()
{
char *spiceOutputBuffer;
int64_t allowedOutputLength;

allowedOutputLength = (int64_t)this->timeOutPicture.size() - 5;
Expand All @@ -499,10 +575,12 @@ std::string SpiceInterface::getCurrentTimeString()
return("");
}

spiceOutputBuffer = new char[allowedOutputLength];
// Use vector instead of raw pointer for Windows compatibility
std::vector<char> spiceOutputBufferVec(allowedOutputLength);
char* spiceOutputBuffer = spiceOutputBufferVec.data();
Comment on lines +579 to +580
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand you're using std::vector to handle the memory of the underlying char*. This might just be personal preference, but I think:

std::unique_ptr<char[]> spiceOutputBuffer(new char[allowedOutputLength]);

is a bit more expressive and safe. Just a thought.

Maybe actually we should make allowedOutputLength a constexpr size_t so that we can simply do:

char spiceOutputBuffer[allowedOutputLength];

and thus we get rid of nasty dynamically allocated memory.


timout_c(this->J2000Current, this->timeOutPicture.c_str(), (SpiceInt) allowedOutputLength,
spiceOutputBuffer);
std::string returnTimeString = spiceOutputBuffer;
delete[] spiceOutputBuffer;
return(returnTimeString);
}
7 changes: 5 additions & 2 deletions src/simulation/environment/spiceInterface/spiceInterface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

#include <vector>
#include <map>
#include <mutex>
#include "architecture/_GeneralModuleFiles/sys_model.h"
#include "architecture/utilities/linearAlgebra.h"
#include "architecture/utilities/bskLogging.h"
Expand All @@ -40,7 +41,7 @@ class SpiceInterface: public SysModel {
public:
SpiceInterface();
~SpiceInterface();

void UpdateState(uint64_t CurrentSimNanos);
int loadSpiceKernel(char *kernelName, const char *dataPath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should change these methods into:

int loadSpiceKernel(std::string_view kernelName, std::string_view dataPath);
int unloadSpiceKernel(std::string_view kernelName, std::string_view dataPath);

or at least std::string. There's no reason why we should be passing char*.

This will also save you a lot of headache on the implementation (see later comment).

int unloadSpiceKernel(char *kernelName, const char *dataPath);
Expand Down Expand Up @@ -72,7 +73,7 @@ class SpiceInterface: public SysModel {
std::string UTCCalInit; //!< -- UTC time string for init time

std::vector<std::string>planetFrames; //!< -- Optional vector of planet frame names. Default values are IAU_ + planet name

bool timeDataInit; //!< -- Flag indicating whether time has been init
double J2000ETInit; //!< s Seconds elapsed since J2000 at init
double J2000Current; //!< s Current J2000 elapsed time
Expand All @@ -90,6 +91,8 @@ class SpiceInterface: public SysModel {
std::vector<SpicePlanetStateMsgPayload> planetData;
std::vector<SpicePlanetStateMsgPayload> scData;

static std::mutex kernel_manipulation_mutex; //!< -- Mutex to protect SPICE kernel loading/unloading operations
static std::map<std::string, int> kernel_reference_counter; //!< -- Reference counter for SPICE kernels
Comment on lines +94 to +95
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably make these variables camelCase. (sorry I originally suggested snake_case!).

Comment on lines +94 to +95
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::unordered_map is probably better here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, variables should be lower camel case.

};


Expand Down