diff --git a/pythonfmu/tests/test_integration.py b/pythonfmu/tests/test_integration.py index 001b195..118d1d2 100644 --- a/pythonfmu/tests/test_integration.py +++ b/pythonfmu/tests/test_integration.py @@ -354,3 +354,43 @@ def test_integration_throw_py_error(tmp_path): with pytest.raises(Exception): fmpy.simulate_fmu(str(fmu), stop_time=1.0) + + +@pytest.mark.integration +def test_integration_reinstantiate_same_fmu_keeps_module_dict_alive(tmp_path): + """findClass() must not Py_DECREF the borrowed PyModule_GetDict() result. + + The FMU runs inside this interpreter, so the reference count of the slave + module's __dict__ can be compared with the objects that visibly refer to it. + A stolen reference shows up as a count lower than the referrer count, one per + fmi2Instantiate of the same FMU. + """ + import gc + import sys + + script_file = Path(__file__).parent / "slaves/pythonslave.py" + fmu = FmuBuilder.build_FMU(script_file, dest=tmp_path, needsExecutionTool="false") + md = fmpy.read_model_description(fmu, validate=False) + unzip_dir = fmpy.extract(fmu) + + models = [] + for i in range(3): + model = fmpy.fmi2.FMU2Slave( + guid=md.guid, + unzipDirectory=unzip_dir, + modelIdentifier=md.coSimulation.modelIdentifier, + instanceName=f"instance{i}", + ) + model.instantiate() + models.append(model) + + gc.collect() + module_dict = sys.modules[script_file.stem].__dict__ + referrers = gc.get_referrers(module_dict) + # getrefcount() counts its own argument; every visible referrer must own a reference. + assert sys.getrefcount(module_dict) - 1 >= len(referrers) + + for model in models: + model.terminate() + model.fmi2FreeInstance(model.component) + model.component = None diff --git a/src/pythonfmu/PySlaveInstance.cpp b/src/pythonfmu/PySlaveInstance.cpp index f05c866..a47c5c7 100644 --- a/src/pythonfmu/PySlaveInstance.cpp +++ b/src/pythonfmu/PySlaveInstance.cpp @@ -62,12 +62,10 @@ PyObject* findClass(const std::string& resources, const std::string& moduleName) PyObject* pResult = PyEval_EvalCode(pCode, pGlobals, pLocals); Py_XDECREF(pResult); } else { - PyErr_Print(); // Handle compilation error - Py_Finalize(); - Py_DECREF(pGlobals); + // Compilation failed: leave the exception set so the caller reports it + // through handle_py_exception(). Py_DECREF(pyModule); Py_DECREF(pLocals); - Py_DECREF(pCode); file.close(); return nullptr; } @@ -113,7 +111,7 @@ PyObject* findClass(const std::string& resources, const std::string& moduleName) Py_DECREF(pCode); Py_DECREF(pLocals); Py_DECREF(pyModule); - Py_DECREF(pGlobals); + // Borrowed reference (PyModule_GetDict); do not DECREF. file.close(); Py_DECREF(pyClassName); return pyClass; @@ -206,8 +204,8 @@ class PySlaveInstance : public SlaveInstance void initialize(PyGILState_STATE gilState) { - Py_XDECREF(pInstance_); - Py_XDECREF(pMessages_); + Py_CLEAR(pInstance_); + Py_CLEAR(pMessages_); PyObject* args = PyTuple_New(0); PyObject* kwargs = Py_BuildValue("{ss,ss,sn,si}", @@ -222,10 +220,14 @@ class PySlaveInstance : public SlaveInstance handle_py_exception("[initialize] PyObject_Call", gilState); } pMessages_ = PyObject_CallMethod(pInstance_, "_get_log_queue", nullptr); + if (pMessages_ == nullptr) { + handle_py_exception("[initialize] _get_log_queue", gilState); + } } void SetupExperiment(double startTime, std::optional stop, std::optional tolerance) override { + ensureUsable("[setupExperiment]"); py_safe_run([this, startTime, stop, tolerance](PyGILState_STATE gilState) { PyObject* pyStop = stop ? Py_BuildValue("d", *stop) : (Py_INCREF(Py_None), Py_None); PyObject* pyTol = tolerance ? Py_BuildValue("d", *tolerance) : (Py_INCREF(Py_None), Py_None); @@ -245,6 +247,7 @@ class PySlaveInstance : public SlaveInstance void EnterInitializationMode() override { + ensureUsable("[enterInitializationMode]"); py_safe_run([this](PyGILState_STATE gilState) { auto f = PyObject_CallMethod(pInstance_, "enter_initialization_mode", nullptr); if (f == nullptr) { @@ -257,6 +260,7 @@ class PySlaveInstance : public SlaveInstance void ExitInitializationMode() override { + ensureUsable("[exitInitializationMode]"); py_safe_run([this](PyGILState_STATE gilState) { auto f = PyObject_CallMethod(pInstance_, "exit_initialization_mode", nullptr); if (f == nullptr) { @@ -269,6 +273,7 @@ class PySlaveInstance : public SlaveInstance bool Step(double currentTime, double stepSize) override { + ensureUsable("[doStep]"); bool status; py_safe_run([this, &status, currentTime, stepSize](PyGILState_STATE gilState) { auto f = PyObject_CallMethod(pInstance_, "do_step", "(dd)", currentTime, stepSize); @@ -285,6 +290,7 @@ class PySlaveInstance : public SlaveInstance void Reset() override { + ensureUsable("[reset]"); py_safe_run([this](PyGILState_STATE gilState) { initialize(gilState); }); @@ -292,6 +298,7 @@ class PySlaveInstance : public SlaveInstance void Terminate() override { + ensureUsable("[terminate]"); py_safe_run([this](PyGILState_STATE gilState) { auto f = PyObject_CallMethod(pInstance_, "terminate", nullptr); if (f == nullptr) { @@ -304,6 +311,7 @@ class PySlaveInstance : public SlaveInstance void SetReal(const fmi2ValueReference* vr, std::size_t nvr, const fmi2Real* values) override { + ensureUsable("[setReal]"); py_safe_run([this, &vr, nvr, &values](PyGILState_STATE gilState) { PyObject* vrs = PyList_New(nvr); PyObject* refs = PyList_New(nvr); @@ -325,6 +333,7 @@ class PySlaveInstance : public SlaveInstance void SetInteger(const fmi2ValueReference* vr, std::size_t nvr, const fmi2Integer* values) override { + ensureUsable("[setInteger]"); py_safe_run([this, &vr, nvr, &values](PyGILState_STATE gilState) { PyObject* vrs = PyList_New(nvr); PyObject* refs = PyList_New(nvr); @@ -346,6 +355,7 @@ class PySlaveInstance : public SlaveInstance void SetBoolean(const fmi2ValueReference* vr, std::size_t nvr, const fmi2Boolean* values) override { + ensureUsable("[setBoolean]"); py_safe_run([this, &vr, nvr, &values](PyGILState_STATE gilState) { PyObject* vrs = PyList_New(nvr); PyObject* refs = PyList_New(nvr); @@ -367,6 +377,7 @@ class PySlaveInstance : public SlaveInstance void SetString(const fmi2ValueReference* vr, std::size_t nvr, fmi2String const* values) override { + ensureUsable("[setString]"); py_safe_run([this, &vr, nvr, &values](PyGILState_STATE gilState) { PyObject* vrs = PyList_New(nvr); PyObject* refs = PyList_New(nvr); @@ -388,6 +399,7 @@ class PySlaveInstance : public SlaveInstance void GetReal(const fmi2ValueReference* vr, std::size_t nvr, fmi2Real* values) const override { + ensureUsable("[getReal]"); py_safe_run([this, &vr, nvr, &values](PyGILState_STATE gilState) { PyObject* vrs = PyList_New(nvr); for (int i = 0; i < nvr; i++) { @@ -411,6 +423,7 @@ class PySlaveInstance : public SlaveInstance void GetInteger(const fmi2ValueReference* vr, std::size_t nvr, fmi2Integer* values) const override { + ensureUsable("[getInteger]"); py_safe_run([this, &vr, nvr, &values](PyGILState_STATE gilState) { PyObject* vrs = PyList_New(nvr); for (int i = 0; i < nvr; i++) { @@ -433,6 +446,7 @@ class PySlaveInstance : public SlaveInstance void GetBoolean(const fmi2ValueReference* vr, std::size_t nvr, fmi2Boolean* values) const override { + ensureUsable("[getBoolean]"); py_safe_run([this, &vr, nvr, &values](PyGILState_STATE gilState) { PyObject* vrs = PyList_New(nvr); for (int i = 0; i < nvr; i++) { @@ -455,6 +469,7 @@ class PySlaveInstance : public SlaveInstance void GetString(const fmi2ValueReference* vr, std::size_t nvr, fmi2String* values) const override { + ensureUsable("[getString]"); py_safe_run([this, &vr, nvr, &values](PyGILState_STATE gilState) { clearStrBuffer(); PyObject* vrs = PyList_New(nvr); @@ -479,6 +494,7 @@ class PySlaveInstance : public SlaveInstance void GetFMUstate(fmi2FMUstate& state) override { + ensureUsable("[_get_fmu_state]"); py_safe_run([this, &state](PyGILState_STATE gilState) { auto f = PyObject_CallMethod(pInstance_, "_get_fmu_state", nullptr); if (f == nullptr) { @@ -491,6 +507,7 @@ class PySlaveInstance : public SlaveInstance void SetFMUstate(const fmi2FMUstate& state) override { + ensureUsable("[_set_fmu_state]"); py_safe_run([this, &state](PyGILState_STATE gilState) { auto pyState = reinterpret_cast(state); auto f = PyObject_CallMethod(pInstance_, "_set_fmu_state", "(O)", pyState); @@ -503,6 +520,7 @@ class PySlaveInstance : public SlaveInstance void FreeFMUstate(fmi2FMUstate& state) override { + ensureUsable("[FreeFMUstate]"); py_safe_run([this, &state](PyGILState_STATE gilState) { auto f = reinterpret_cast(state); Py_XDECREF(f); @@ -511,6 +529,7 @@ class PySlaveInstance : public SlaveInstance size_t SerializedFMUstateSize(const fmi2FMUstate& state) override { + ensureUsable("[SerializedFMUstateSize]"); size_t size; py_safe_run([this, &state, &size](PyGILState_STATE gilState) { auto pyState = reinterpret_cast(state); @@ -527,6 +546,7 @@ class PySlaveInstance : public SlaveInstance void SerializeFMUstate(const fmi2FMUstate& state, fmi2Byte* bytes, size_t size) override { + ensureUsable("[SerializeFMUstate]"); py_safe_run([this, &state, &bytes, size](PyGILState_STATE gilState) { auto pyState = reinterpret_cast(state); PyObject* pyStateBytes = PyObject_CallMethod(pClass_, "_fmu_state_to_bytes", "(O)", pyState); @@ -547,6 +567,7 @@ class PySlaveInstance : public SlaveInstance void DeSerializeFMUstate(const fmi2Byte bytes[], size_t size, fmi2FMUstate& state) override { + ensureUsable("[DeSerializeFMUstate]"); py_safe_run([this, &bytes, size, &state](PyGILState_STATE gilState) { PyObject* pyStateBytes = PyBytes_FromStringAndSize(bytes, size); if (pyStateBytes == nullptr) { @@ -572,9 +593,15 @@ class PySlaveInstance : public SlaveInstance private: fmu_data data_; - PyObject* pClass_; - PyObject* pInstance_{}; - PyObject* pMessages_{}; + // Owned references, released with Py_CLEAR so no path can release one twice. + mutable PyObject* pClass_{}; + mutable PyObject* pInstance_{}; + mutable PyObject* pMessages_{}; + + // Set by the first Python exception, whose handler releases the slave object. + // Every later operation fails fast (fmi2Fatal) without touching Python, and the + // destructor (fmi2FreeInstance) only releases what is still held. + mutable bool failed_{false}; mutable std::vector strBuffer; mutable std::vector logStrBuffer; @@ -619,44 +646,51 @@ class PySlaveInstance : public SlaveInstance clearLogBuffer(); clearLogStrBuffer(); clearStrBuffer(); - Py_XDECREF(pClass_); - Py_XDECREF(pInstance_); - Py_XDECREF(pMessages_); + Py_CLEAR(pClass_); + Py_CLEAR(pInstance_); + Py_CLEAR(pMessages_); } - - void handle_py_exception(const std::string& what, PyGILState_STATE gilState) const + void ensureUsable(const char* what) const { - const auto err = PyErr_Occurred(); - if (err != nullptr) { - cleanPyObject(); + if (failed_) { + throw fatal_error(std::string(what) + + " refused: an earlier Python exception was fatal and released this " + "instance's slave object; only fmi2FreeInstance is valid now"); + } + } - PyObject *pExcType, *pExcValue, *pExcTraceback; - PyErr_Fetch(&pExcType, &pExcValue, &pExcTraceback); + // Never returns: marks this instance failed, releases its Python objects and the + // GIL taken by py_safe_run, and throws fatal_error (reported as fmi2Fatal). + [[noreturn]] void handle_py_exception(const std::string& what, PyGILState_STATE gilState) const + { + failed_ = true; - std::ostringstream oss; - oss << "Fatal py exception encountered: "; - oss << what << "\n"; - if (pExcValue != nullptr) { - PyObject* pRepr = PyObject_Repr(pExcValue); - PyObject* pyStr = PyUnicode_AsEncodedString(pRepr, "utf-8", nullptr); - oss << PyBytes_AsString(pyStr); - Py_DECREF(pyStr); - Py_DECREF(pRepr); - } else { - oss << "unknown error"; - } + std::ostringstream oss; + oss << "Fatal py exception encountered: "; + oss << what << "\n"; - PyErr_Clear(); + // Take the pending exception first, so the cleanup below never runs Python + // code (log flushing, __del__) with an exception already set. + PyObject *pExcType, *pExcValue, *pExcTraceback; + PyErr_Fetch(&pExcType, &pExcValue, &pExcTraceback); + PyObject* pRepr = pExcValue != nullptr ? PyObject_Repr(pExcValue) : nullptr; + PyObject* pyStr = pRepr != nullptr ? PyUnicode_AsEncodedString(pRepr, "utf-8", nullptr) : nullptr; + const char* text = pyStr != nullptr ? PyBytes_AsString(pyStr) : nullptr; + oss << (text != nullptr ? text : "unknown error"); + Py_XDECREF(pyStr); + Py_XDECREF(pRepr); + Py_XDECREF(pExcType); + Py_XDECREF(pExcValue); + Py_XDECREF(pExcTraceback); + PyErr_Clear(); // a failed repr/encode above - Py_XDECREF(pExcType); - Py_XDECREF(pExcValue); - Py_XDECREF(pExcTraceback); + cleanPyObject(); + PyErr_Clear(); // anything the cleanup raised - PyGILState_Release(gilState); + PyGILState_Release(gilState); - throw fatal_error(oss.str()); - } + throw fatal_error(oss.str()); } }; @@ -664,21 +698,26 @@ namespace { std::mutex pyStateMutex{}; -std::shared_ptr pyState{}; +// Intentionally heap-allocated and never deleted. A namespace-scope +// std::shared_ptr is destroyed by its static destructor at process exit +// and released again by the platform unload hook; the order of those two is +// unspecified, so the second release read a freed control block +// (heap-use-after-free -> "corrupted double-linked list" at exit). +std::shared_ptr* pyState = new std::shared_ptr(); } // namespace std::unique_ptr pythonfmu::createInstance(fmu_data data) { { - auto const ensurePyStateAlive = [&]() { + auto const ensurePyStateAlive = [&]() -> std::shared_ptr { auto const lock = std::lock_guard{pyStateMutex}; - if (nullptr == pyState) pyState = std::make_shared(); + if (nullptr == *pyState) *pyState = std::make_shared(); + return *pyState; }; - ensurePyStateAlive(); + data.pyState = ensurePyStateAlive(); auto c = std::make_unique(data); - data.pyState = pyState; return c; } } @@ -689,10 +728,11 @@ extern "C" { // The PyState instance owns it's own thread for constructing and destroying the Py* from the same thread. // Creation of an std::thread increments ref counter of a shared library. So, when the client unloads the library // the library won't be freed, as std::thread is alive, and the std::thread itself waits for de-initialization request. -// Thus, use DllMain on Windows and __attribute__((destructor)) on Linux for signaling to the PyState about de-initialization. +// Thus, use the platform unload hook to signal PyState about de-initialization. void finalizePythonInterpreter() { - pyState = nullptr; + auto const lock = std::lock_guard{pyStateMutex}; + pyState->reset(); } }