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

Natsoulas · 2025-05-07T05:36:43Z

Tickets addressed: Spice makes BSK simulations not thread safe #220
Review: By commit

Description

The changes implement thread safety for the SPICE interface to address GitHub issue #220, which reported deadlocks and data corruption when running parallel Monte Carlo simulations.

This solution uses a mutex and reference counting approach to protect SPICE kernel loading and unloading operations (inspired by Juan's suggestion comment on the issue):

Added a static mutex kernel_manipulation_mutex in the SpiceInterface class
Added a static reference counter kernel_reference_counter to track kernel usage
Modified loadSpiceKernel() to only load kernels once and use reference counting
Modified unloadSpiceKernel() to only unload kernels when no longer needed
Protected clearKeeper() with the mutex
Updated the destructor to properly unload kernels

This ensures that when multiple simulations run in parallel, they won't encounter race conditions or deadlocks when accessing SPICE kernels.

Verification

The changes were verified with a dedicated thread safety unit test:

Added test_spiceThreadSafety.py unit test in the SPICE interface's _UnitTest directory
The test creates multiple SpiceInterface instances in parallel and forces them to simultaneously load/unload kernels
It verifies no deadlocks occur and no kernel files are corrupted
No existing functionality was broken - all other SPICE-related tests continue to pass

The test specifically reproduces the conditions described in issue #220.

Documentation

Added code comments explaining the thread safety mechanism in:

spiceInterface.h for the static mutex and reference counter
spiceInterface.cpp for the mutex implementation and kernel reference counting logic

No existing documentation was invalidated.

Future work

Monitor for the release of NAIF's thread-safe SPICE implementation, which may provide a better solution in the future (Juan's advice from a comment in the issue 220 page).

src/simulation/environment/spiceInterface/_UnitTest/test_spiceThreadSafety.py

schaubh

nice work! All tests passed on my system. Just some minor comments to address

schaubh · 2025-05-13T17:08:22Z

src/simulation/environment/spiceInterface/_UnitTest/test_spiceThreadSafety.py

+import sys
+import time
+import hashlib
+import random


random is not used, please delete this import

schaubh · 2025-05-13T17:08:42Z

src/simulation/environment/spiceInterface/_UnitTest/test_spiceThreadSafety.py

+import multiprocessing as mp
+import pytest
+import inspect
+import numpy as np


numpy is not used, please delete this import

schaubh · 2025-05-13T17:09:15Z

src/simulation/environment/spiceInterface/_UnitTest/test_spiceThreadSafety.py

+import inspect
+import numpy as np
+import traceback
+import signal


signal is imported, if needed, later on. We can delete this import.

schaubh · 2025-05-13T17:09:37Z

src/simulation/environment/spiceInterface/_UnitTest/test_spiceThreadSafety.py

+# Import Basilisk modules
+from Basilisk import __path__
+from Basilisk.simulation import spiceInterface
+from Basilisk.utilities import unitTestSupport


You are not using unitTestSupport, we can delete this import

schaubh · 2025-05-13T17:10:48Z

src/simulation/environment/spiceInterface/_UnitTest/test_spiceThreadSafety.py

+===========================================
+
+This script tests the thread safety of the SPICE interface, specifically addressing
+GitHub issue #220 where parallel simulations using SPICE were causing deadlocks and


make this part of the documentation a hyperlink to issue 220?

schaubh · 2025-05-13T17:13:14Z

Nice work, looks like we are there. @juan-g-bonilla , as you commented on the original issue with the suggestion to use a mutex, would you have time to take a look at this code?

juan-g-bonilla

Nicely done @Natsoulas ! Many of my comments are actually encouraging you to fix existing issues or even the original fix I proposed so we can get this class to its highest quality possible.

Thanks for the tag @schaubh .

juan-g-bonilla · 2025-05-13T17:24:49Z

src/simulation/environment/spiceInterface/spiceInterface.h

+    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


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

juan-g-bonilla · 2025-05-13T17:25:14Z

src/simulation/environment/spiceInterface/spiceInterface.h

+    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


std::unordered_map is probably better here.

yes, variables should be lower camel case.

juan-g-bonilla · 2025-05-13T17:30:27Z

src/simulation/environment/spiceInterface/spiceInterface.h

@@ -40,7 +41,7 @@ class SpiceInterface: public SysModel {
 public:
    SpiceInterface();
    ~SpiceInterface();
-    
+
    void UpdateState(uint64_t CurrentSimNanos);
    int loadSpiceKernel(char *kernelName, const char *dataPath);


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).

juan-g-bonilla · 2025-05-13T17:32:54Z

src/simulation/environment/spiceInterface/spiceInterface.cpp

+        // 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());


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

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

juan-g-bonilla · 2025-05-13T17:39:40Z

src/simulation/environment/spiceInterface/spiceInterface.cpp

+        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");
+


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.

True, this is a cleaner solution.

juan-g-bonilla · 2025-05-13T17:41:32Z

src/simulation/environment/spiceInterface/spiceInterface.cpp

+    // 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);


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

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

juan-g-bonilla · 2025-05-13T17:42:11Z

src/simulation/environment/spiceInterface/spiceInterface.cpp

+    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;
+        }


Nice job fetching the error from SPICE.

juan-g-bonilla · 2025-05-13T17:42:33Z

src/simulation/environment/spiceInterface/spiceInterface.cpp

+    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);


Similar comment as above.

juan-g-bonilla · 2025-05-13T17:51:06Z

src/simulation/environment/spiceInterface/spiceInterface.cpp

+	std::vector<char> spiceOutputBufferVec(allowedOutputLength);
+	char* spiceOutputBuffer = spiceOutputBufferVec.data();


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.

juan-g-bonilla · 2025-05-13T17:53:21Z

src/simulation/environment/spiceInterface/_UnitTest/test_spiceThreadSafety.py

@@ -0,0 +1,339 @@
+#


Very comprehensive test, nicely done.

Just for own curiosity, have you run this test without your fix? We would want it to fail on that case, so that we're sure the test is actually correctly stressing the system. I remember this error is infamously tricky to reproduce, so we want to make sure this test is actually testing as we need.

Natsoulas requested a review from schaubh May 7, 2025 05:36

Natsoulas self-assigned this May 7, 2025

Natsoulas requested a review from a team as a code owner May 7, 2025 05:36

Natsoulas added bug Something isn't working enhancement New feature or request labels May 7, 2025

Natsoulas linked an issue May 7, 2025 that may be closed by this pull request

Spice makes BSK simulations not thread safe #220

Open

Natsoulas added this to Basilisk May 7, 2025

Natsoulas moved this to 🏗 In progress in Basilisk May 7, 2025

Natsoulas force-pushed the bug/220-spice-makes-bsk-sims-not-thread-safe branch from 4a1af4d to aeed048 Compare May 7, 2025 06:10

schaubh reviewed May 7, 2025

View reviewed changes

src/simulation/environment/spiceInterface/_UnitTest/test_spiceThreadSafety.py Outdated Show resolved Hide resolved

Natsoulas force-pushed the bug/220-spice-makes-bsk-sims-not-thread-safe branch 2 times, most recently from def2f18 to 8f6bf30 Compare May 9, 2025 03:35

Natsoulas added 3 commits May 10, 2025 12:18

Mutex for thread safety and ref counting

d3d7467

Add new test for spice thread safety

7389555

Update Release Notes

a9ef12b

Natsoulas force-pushed the bug/220-spice-makes-bsk-sims-not-thread-safe branch from 8f6bf30 to a9ef12b Compare May 10, 2025 18:18

Natsoulas moved this from 🏗 In progress to 👀 In review in Basilisk May 10, 2025

schaubh requested changes May 13, 2025

View reviewed changes

schaubh requested a review from juan-g-bonilla May 13, 2025 17:12

juan-g-bonilla requested changes May 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Natsoulas commented May 7, 2025

schaubh left a comment

schaubh May 13, 2025

schaubh May 13, 2025

schaubh May 13, 2025

schaubh May 13, 2025

schaubh May 13, 2025

schaubh commented May 13, 2025

juan-g-bonilla left a comment •

edited

Loading

juan-g-bonilla May 13, 2025

juan-g-bonilla May 13, 2025

schaubh May 13, 2025

juan-g-bonilla May 13, 2025

juan-g-bonilla May 13, 2025

juan-g-bonilla May 13, 2025

schaubh May 13, 2025

juan-g-bonilla May 13, 2025

juan-g-bonilla May 13, 2025

juan-g-bonilla May 13, 2025

juan-g-bonilla May 13, 2025

juan-g-bonilla May 13, 2025

		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

		std::vector<char> spiceOutputBufferVec(allowedOutputLength);
		char* spiceOutputBuffer = spiceOutputBufferVec.data();

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

Are you sure you want to change the base?

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

Conversation

Natsoulas commented May 7, 2025

Description

Verification

Documentation

Future work

schaubh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schaubh commented May 13, 2025

juan-g-bonilla left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juan-g-bonilla left a comment •

edited

Loading