CPythonInvoker: code cleanup #22412
CPythonInvoker: code cleanup #22412
Conversation
|
|
||
| if (Py_IsInitialized()) | ||
| { | ||
| PyThreadState_Swap(PyInterpreterState_ThreadHead(PyInterpreterState_Main())); |
There was a problem hiding this comment.
PyInterpreterState_Main() requires Python >= 3.7. Is that dependency acceptable?
Test build on openSUSE Leap 15.4 with Python 3.6 is failing.
There was a problem hiding this comment.
You are right this function was already implemented (PythonInvoker.cpp) but with a version check. I will update the code.
There was a problem hiding this comment.
Thanks, is compiling and starting (no further tests yet).
| { | ||
| for (const auto& module : modules) | ||
| for (int i = 0; i < numElements; i++) |
There was a problem hiding this comment.
any reason for removing the range for loop?
There was a problem hiding this comment.
This construct operates locally and can't be used to transfer the size of the array to the base class.
| { | ||
| if (module == NULL) | ||
| return false; | ||
|
|
||
| return module() != nullptr; | ||
| return !PyImport_AppendInittab(name, module); |
There was a problem hiding this comment.
While looking at #22344 I've noticed that inittab is a global resource in Python. Each module should only be added once. Modifying the inittab later may even crash.
My current proof proof concept (without this PR) is:
From dc07e8b90821896b4d529fb6abb57d04e7fac88f Mon Sep 17 00:00:00 2001
From: mglae <mglmail@arcor.de>
Date: Thu, 5 Jan 2023 00:35:22 +0100
Subject: [PATCH] AddonPythonInvoker: Only call PyImport_AppendInittab() once
The modules are added to a single Inittab so
a) The tab will grow with any call and duplicate entries are added.
b) The tab check code may crash while the tab is expanded.
---
xbmc/interfaces/python/AddonPythonInvoker.cpp | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/xbmc/interfaces/python/AddonPythonInvoker.cpp b/xbmc/interfaces/python/AddonPythonInvoker.cpp
index b6158a8db6..34280f48ac 100644
--- a/xbmc/interfaces/python/AddonPythonInvoker.cpp
+++ b/xbmc/interfaces/python/AddonPythonInvoker.cpp
@@ -103,12 +103,18 @@ static PythonModule PythonModules[] =
CAddonPythonInvoker::CAddonPythonInvoker(ILanguageInvocationHandler *invocationHandler)
: CPythonInvoker(invocationHandler)
{
- PyImport_AppendInittab("xbmcdrm", PyInit_Module_xbmcdrm);
- PyImport_AppendInittab("xbmcgui", PyInit_Module_xbmcgui);
- PyImport_AppendInittab("xbmc", PyInit_Module_xbmc);
- PyImport_AppendInittab("xbmcplugin", PyInit_Module_xbmcplugin);
- PyImport_AppendInittab("xbmcaddon", PyInit_Module_xbmcaddon);
- PyImport_AppendInittab("xbmcvfs", PyInit_Module_xbmcvfs);
+ static bool first_init(true);
+
+ if (first_init)
+ {
+ first_init = false;
+ PyImport_AppendInittab("xbmcdrm", PyInit_Module_xbmcdrm);
+ PyImport_AppendInittab("xbmcgui", PyInit_Module_xbmcgui);
+ PyImport_AppendInittab("xbmc", PyInit_Module_xbmc);
+ PyImport_AppendInittab("xbmcplugin", PyInit_Module_xbmcplugin);
+ PyImport_AppendInittab("xbmcaddon", PyInit_Module_xbmcaddon);
+ PyImport_AppendInittab("xbmcvfs", PyInit_Module_xbmcvfs);
+ }
}
CAddonPythonInvoker::~CAddonPythonInvoker() = default;
--
2.35.3
There was a problem hiding this comment.
The current code seems to implement a kind of "sub interpreter". This feature is not properly documented, and requires the function PyImport_AppendInittab() to be called locally. This new implementation doesn't change the previous behavior.
Here is an implementation of a "sub interpreter": https://gist.github.com/sterin/61561c3139dd49da1f43
It could be relevant to check #22344 with the gcc sanitizer (kodi + the python library).
There was a problem hiding this comment.
The current code seems to implement a kind of "sub interpreter". This feature is not properly documented, and requires the function PyImport_AppendInittab() to be called locally.
PyImport_AppendInittab() modifies a global table and should be called before Py_Initialize(), see comment and implementation here.
There was a problem hiding this comment.
PyImport_AppendInittab() modifies a global table and should be called before Py_Initialize(), see comment and implementation here.
I tried, this is described in the python documentation, but the "sub interpreter" was not working anymore. This new implementation would log the PyImport_AppendInittab() errors which are not happening.
There was a problem hiding this comment.
The #22344 issue seems to be related to thread safety, I doubt changing PyImport_AppendInittab() would fix anything, but the gcc sanitizer could help to find where the issue is.
There was a problem hiding this comment.
#22344 is a Python regression and will be fixed in the next 3.10/3.11 dot releases.
But while testing I noticed the PyImport_AppendInittab() related crashes.
The following patch to this PR is working fine in my Linux builds with external Python 3.6.15 and 3.11.1:
diff --git a/xbmc/interfaces/python/AddonPythonInvoker.cpp b/xbmc/interfaces/python/AddonPythonInvoker.cpp
index ca6b732af3..ce36309513 100644
--- a/xbmc/interfaces/python/AddonPythonInvoker.cpp
+++ b/xbmc/interfaces/python/AddonPythonInvoker.cpp
@@ -103,7 +103,13 @@ const std::array<PythonModule, 6> PythonModules =
CAddonPythonInvoker::CAddonPythonInvoker(ILanguageInvocationHandler *invocationHandler)
: CPythonInvoker(invocationHandler)
{
- initializeModules(PythonModules);
+ static bool first_init(true);
+
+ if (first_init)
+ {
+ first_init = false;
+ initializeModules(PythonModules);
+ }
}
CAddonPythonInvoker::~CAddonPythonInvoker() = default;
There was a problem hiding this comment.
@mglae, your patch is working, but we should keep in mind the official python documentation PyImport_AppendInittab(): - "This should be called before Py_Initialize()" https://docs.python.org/3/c-api/import.html
Here is a patch compliant with the python documentation:
diff --git a/xbmc/interfaces/python/AddonPythonInvoker.cpp b/xbmc/interfaces/python/AddonPythonInvoker.cpp
index ca6b732af3..b360c7d45d 100644
--- a/xbmc/interfaces/python/AddonPythonInvoker.cpp
+++ b/xbmc/interfaces/python/AddonPythonInvoker.cpp
@@ -103,11 +103,15 @@ const std::array<PythonModule, 6> PythonModules =
CAddonPythonInvoker::CAddonPythonInvoker(ILanguageInvocationHandler *invocationHandler)
: CPythonInvoker(invocationHandler)
{
- initializeModules(PythonModules);
}
CAddonPythonInvoker::~CAddonPythonInvoker() = default;
+void CAddonPythonInvoker::globalInitializeModules(void)
+{
+ initializeModules(PythonModules);
+}
+
const char* CAddonPythonInvoker::getInitializationScript() const
{
return RUNSCRIPT_COMPLIANT;
diff --git a/xbmc/interfaces/python/AddonPythonInvoker.h b/xbmc/interfaces/python/AddonPythonInvoker.h
index 5ba0786438..cacf144ddd 100644
--- a/xbmc/interfaces/python/AddonPythonInvoker.h
+++ b/xbmc/interfaces/python/AddonPythonInvoker.h
@@ -16,6 +16,8 @@ public:
explicit CAddonPythonInvoker(ILanguageInvocationHandler *invocationHandler);
~CAddonPythonInvoker() override;
+ static void globalInitializeModules(void);
+
protected:
// overrides of CPythonInvoker
const char* getInitializationScript() const override;
diff --git a/xbmc/interfaces/python/PythonInvoker.h b/xbmc/interfaces/python/PythonInvoker.h
index 4583e1629a..09b68c2f0e 100644
--- a/xbmc/interfaces/python/PythonInvoker.h
+++ b/xbmc/interfaces/python/PythonInvoker.h
@@ -60,18 +60,18 @@ protected:
protected:
template<size_t NUMBER>
- void initializeModules(const std::array<PythonModule, NUMBER> modules)
+ static void initializeModules(const std::array<PythonModule, NUMBER> modules)
{
for (const auto& module : modules)
{
if (!initializeModule(module.name, module.initialization))
- CLog::Log(LOGWARNING, "CPythonInvoker({}, {}): unable to initialize python module \"{}\"",
- GetId(), m_sourceFile, module.name);
+ CLog::Log(LOGWARNING, "CPythonInvoker(): unable to initialize python module \"{}\"",
+ module.name);
}
}
private:
- bool initializeModule(const char* name, PythonModuleInitialization module);
+ static bool initializeModule(const char* name, PythonModuleInitialization module);
void getAddonModuleDeps(const ADDON::AddonPtr& addon, std::set<std::string>& paths);
bool execute(const std::string& script, std::vector<std::wstring>& arguments);
FILE* PyFile_AsFileWithMode(PyObject* py_file, const char* mode);
diff --git a/xbmc/interfaces/python/XBPython.cpp b/xbmc/interfaces/python/XBPython.cpp
index 9cb9ba00ac..86079d1d28 100644
--- a/xbmc/interfaces/python/XBPython.cpp
+++ b/xbmc/interfaces/python/XBPython.cpp
@@ -34,6 +34,10 @@
#include "platform/Environment.h"
#endif
+#ifdef HAS_WEB_INTERFACE
+#include "network/httprequesthandler/python/HTTPPythonWsgiInvoker.h"
+#endif
+
#include <algorithm>
// Only required for Py3 < 3.7
@@ -526,6 +530,12 @@ bool XBPython::OnScriptInitialized(ILanguageInvoker* invoker)
Py_OptimizeFlag = 1;
#endif
+ CAddonPythonInvoker::globalInitializeModules();
+
+#ifdef HAS_WEB_INTERFACE
+ CHTTPPythonWsgiInvoker::globalInitializeModules();
+#endif
+
Py_Initialize();
#if PY_VERSION_HEX < 0x03070000
diff --git a/xbmc/network/httprequesthandler/python/HTTPPythonWsgiInvoker.cpp b/xbmc/network/httprequesthandler/python/HTTPPythonWsgiInvoker.cpp
index 3bc46c1b76..1dd3f2d33a 100644
--- a/xbmc/network/httprequesthandler/python/HTTPPythonWsgiInvoker.cpp
+++ b/xbmc/network/httprequesthandler/python/HTTPPythonWsgiInvoker.cpp
@@ -92,7 +92,6 @@ CHTTPPythonWsgiInvoker::CHTTPPythonWsgiInvoker(ILanguageInvocationHandler* invoc
: CHTTPPythonInvoker(invocationHandler, request),
m_wsgiResponse(NULL)
{
- initializeModules(PythonModules);
}
CHTTPPythonWsgiInvoker::~CHTTPPythonWsgiInvoker()
@@ -101,6 +100,11 @@ CHTTPPythonWsgiInvoker::~CHTTPPythonWsgiInvoker()
m_wsgiResponse = NULL;
}
+void CHTTPPythonWsgiInvoker::globalInitializeModules(void)
+{
+ initializeModules(PythonModules);
+}
+
HTTPPythonRequest* CHTTPPythonWsgiInvoker::GetRequest()
{
if (m_request == NULL || m_wsgiResponse == NULL)
diff --git a/xbmc/network/httprequesthandler/python/HTTPPythonWsgiInvoker.h b/xbmc/network/httprequesthandler/python/HTTPPythonWsgiInvoker.h
index 73f9ccf0be..86f075e517 100644
--- a/xbmc/network/httprequesthandler/python/HTTPPythonWsgiInvoker.h
+++ b/xbmc/network/httprequesthandler/python/HTTPPythonWsgiInvoker.h
@@ -29,6 +29,8 @@ public:
CHTTPPythonWsgiInvoker(ILanguageInvocationHandler* invocationHandler, HTTPPythonRequest* request);
~CHTTPPythonWsgiInvoker() override;
+ static void globalInitializeModules(void);
+
// implementations of CHTTPPythonInvoker
HTTPPythonRequest* GetRequest() override;
There was a problem hiding this comment.
@repojohnray Agree, this is the correct way and it's working fine.
I can confirm this commit fixes the memory leaks.
There was a problem hiding this comment.
@mglae, I will update the commit.
repojohnray
force-pushed
the
cpythoninvokercodecleanup
branch
from
755055f
to
801127f
Compare
|
The commit is updated. |
|
I suggest to convert the c-style arrays to diff --git a/xbmc/interfaces/python/AddonPythonInvoker.cpp b/xbmc/interfaces/python/AddonPythonInvoker.cpp
index 399593b81b..ca6b732af3 100644
--- a/xbmc/interfaces/python/AddonPythonInvoker.cpp
+++ b/xbmc/interfaces/python/AddonPythonInvoker.cpp
@@ -9,6 +9,7 @@
// python.h should always be included first before any other includes
#include "AddonPythonInvoker.h"
+#include <array>
#include <utility>
#include <Python.h>
@@ -87,14 +88,14 @@ using namespace PythonBindings;
namespace
{
// clang-format off
-const PythonModule PythonModules[] =
+const std::array<PythonModule, 6> PythonModules =
{
- { "xbmcdrm", PyInit_Module_xbmcdrm },
- { "xbmcgui", PyInit_Module_xbmcgui },
- { "xbmc", PyInit_Module_xbmc },
- { "xbmcplugin", PyInit_Module_xbmcplugin },
- { "xbmcaddon", PyInit_Module_xbmcaddon },
- { "xbmcvfs", PyInit_Module_xbmcvfs }
+ PythonModule{ "xbmcdrm", PyInit_Module_xbmcdrm },
+ PythonModule{ "xbmcgui", PyInit_Module_xbmcgui },
+ PythonModule{ "xbmc", PyInit_Module_xbmc },
+ PythonModule{ "xbmcplugin", PyInit_Module_xbmcplugin },
+ PythonModule{ "xbmcaddon", PyInit_Module_xbmcaddon },
+ PythonModule{ "xbmcvfs", PyInit_Module_xbmcvfs }
};
// clang-format on
} // namespace
@@ -102,7 +103,7 @@ const PythonModule PythonModules[] =
CAddonPythonInvoker::CAddonPythonInvoker(ILanguageInvocationHandler *invocationHandler)
: CPythonInvoker(invocationHandler)
{
- initializeModules(PythonModules, sizeof(PythonModules) / sizeof(*PythonModules));
+ initializeModules(PythonModules);
}
CAddonPythonInvoker::~CAddonPythonInvoker() = default;
diff --git a/xbmc/interfaces/python/PythonInvoker.cpp b/xbmc/interfaces/python/PythonInvoker.cpp
index 7e57ad92b7..32b9089d8d 100644
--- a/xbmc/interfaces/python/PythonInvoker.cpp
+++ b/xbmc/interfaces/python/PythonInvoker.cpp
@@ -678,16 +678,6 @@ void CPythonInvoker::onError(const std::string& exceptionType /* = "" */,
}
}
-void CPythonInvoker::initializeModules(const PythonModule modules[], const int numElements)
-{
- for (int i = 0; i < numElements; i++)
- {
- if (!initializeModule(modules[i].name, modules[i].initialization))
- CLog::Log(LOGWARNING, "CPythonInvoker({}, {}): unable to initialize python module \"{}\"",
- GetId(), m_sourceFile, modules[i].name);
- }
-}
-
bool CPythonInvoker::initializeModule(const char* name, PythonModuleInitialization module)
{
if (module == NULL)
diff --git a/xbmc/interfaces/python/PythonInvoker.h b/xbmc/interfaces/python/PythonInvoker.h
index 3018d1bf82..4583e1629a 100644
--- a/xbmc/interfaces/python/PythonInvoker.h
+++ b/xbmc/interfaces/python/PythonInvoker.h
@@ -59,7 +59,16 @@ protected:
CCriticalSection m_critical;
protected:
- void initializeModules(const PythonModule modules[], const int numElements);
+ template<size_t NUMBER>
+ void initializeModules(const std::array<PythonModule, NUMBER> modules)
+ {
+ for (const auto& module : modules)
+ {
+ if (!initializeModule(module.name, module.initialization))
+ CLog::Log(LOGWARNING, "CPythonInvoker({}, {}): unable to initialize python module \"{}\"",
+ GetId(), m_sourceFile, module.name);
+ }
+ }
private:
bool initializeModule(const char* name, PythonModuleInitialization module);
diff --git a/xbmc/network/httprequesthandler/python/HTTPPythonWsgiInvoker.cpp b/xbmc/network/httprequesthandler/python/HTTPPythonWsgiInvoker.cpp
index c617ba54e9..3bc46c1b76 100644
--- a/xbmc/network/httprequesthandler/python/HTTPPythonWsgiInvoker.cpp
+++ b/xbmc/network/httprequesthandler/python/HTTPPythonWsgiInvoker.cpp
@@ -19,6 +19,7 @@
#include "utils/StringUtils.h"
#include "utils/URIUtils.h"
+#include <array>
#include <utility>
#include <Python.h>
@@ -78,11 +79,11 @@ using namespace PythonBindings;
namespace
{
// clang-format off
-const PythonModule PythonModules[] =
+const std::array<PythonModule, 3> PythonModules =
{
- { "xbmc", PyInit_Module_xbmc },
- { "xbmcaddon", PyInit_Module_xbmcaddon },
- { "xbmcwsgi", PyInit_Module_xbmcwsgi }
+ PythonModule{ "xbmc", PyInit_Module_xbmc },
+ PythonModule{ "xbmcaddon", PyInit_Module_xbmcaddon },
+ PythonModule{ "xbmcwsgi", PyInit_Module_xbmcwsgi }
};
// clang-format on
} // namespace
@@ -91,7 +92,7 @@ CHTTPPythonWsgiInvoker::CHTTPPythonWsgiInvoker(ILanguageInvocationHandler* invoc
: CHTTPPythonInvoker(invocationHandler, request),
m_wsgiResponse(NULL)
{
- initializeModules(PythonModules, sizeof(PythonModules) / sizeof(*PythonModules));
+ initializeModules(PythonModules);
}
CHTTPPythonWsgiInvoker::~CHTTPPythonWsgiInvoker()Other than that I don't experience issues at runtime and it definitely reduces the number of leaks on exit 😃. |
|
@neo1973, this is not a good idea, PyImport_AppendInittab() requires a valid name until the end of the session (c_str() like). This was previously implemented with a direct call to PyImport_AppendInittab() at the constructor level; Some dead code was also using "static std::map<std::string, PythonModuleInitialization> modules;" a solution that is not thread safe. The current code lets the linker processes everything, this is the fastest and optimal solution. |
|
@repojohnray: Sorry, I meant to pass the |
|
The c_str() reference of the std::string (if we want to use std::string with the std::array) would likely be deleted before the end of the session, this would crash the python "sub interpreter". The main issue here is not to use any dynamically allocated object. |
|
I don't want to change to code to use std::string, I just want to get rid of the c-arrays and the ugly length calculations it requires. Also being able to use ranged for loops is nicer. |
|
@neo1973, OK, I check with the sanitizer. |
|
@neo1973, your patch is working flawlessly. I update the commit. |
repojohnray
force-pushed
the
cpythoninvokercodecleanup
branch
from
801127f
to
4b4057c
Compare
wsnipex
added
Type: Fix
repojohnray
force-pushed
the
cpythoninvokercodecleanup
branch
from
4b4057c
to
83e983c
Compare
|
The commit is updated to call PyImport_AppendInittab() before Py_Initialize() as required by the Python documentation. |
| { "xbmc", PyInit_Module_xbmc }, | ||
| { "xbmcaddon", PyInit_Module_xbmcaddon }, | ||
| { "xbmcwsgi", PyInit_Module_xbmcwsgi } | ||
| PythonModule{ "xbmc", PyInit_Module_xbmc }, |
There was a problem hiding this comment.
explicit type shouldn't be needed
| PythonModule{ "xbmc", PyInit_Module_xbmc }, | |
| { "xbmc", PyInit_Module_xbmc }, |
There was a problem hiding this comment.
The last patch by @neo1973 simplifies this issue.
| @@ -29,13 +29,14 @@ class CHTTPPythonWsgiInvoker : public CHTTPPythonInvoker | |||
| CHTTPPythonWsgiInvoker(ILanguageInvocationHandler* invocationHandler, HTTPPythonRequest* request); | |||
| ~CHTTPPythonWsgiInvoker() override; | |||
|
|
|||
| static void globalInitializeModules(void); | |||
There was a problem hiding this comment.
| static void globalInitializeModules(void); | |
| static void GlobalInitializeModules(); |
ref: https://github.com/xbmc/xbmc/blob/master/docs/CODE_GUIDELINES.md#86-methods
and: https://github.com/xbmc/xbmc/blob/master/docs/CODE_GUIDELINES.md#36-superfluous-void
There was a problem hiding this comment.
@lrusak, you are right this is updated in the last commit.
| @@ -16,8 +16,9 @@ class CAddonPythonInvoker : public CPythonInvoker | |||
| explicit CAddonPythonInvoker(ILanguageInvocationHandler *invocationHandler); | |||
| ~CAddonPythonInvoker() override; | |||
|
|
|||
| static void globalInitializeModules(void); | |||
There was a problem hiding this comment.
| static void globalInitializeModules(void); | |
| static void GlobalInitializeModules(); |
ref: https://github.com/xbmc/xbmc/blob/master/docs/CODE_GUIDELINES.md#86-methods
and: https://github.com/xbmc/xbmc/blob/master/docs/CODE_GUIDELINES.md#36-superfluous-void
There was a problem hiding this comment.
@lrusak, this is updated in the last commit.
| @@ -58,9 +58,20 @@ class CPythonInvoker : public ILanguageInvoker | |||
| std::string m_sourceFile; | |||
| CCriticalSection m_critical; | |||
|
|
|||
| protected: | |||
| template<size_t NUMBER> | |||
| static void initializeModules(const std::array<PythonModule, NUMBER> modules) | |||
There was a problem hiding this comment.
| static void initializeModules(const std::array<PythonModule, NUMBER> modules) | |
| static void InitializeModules(const std::array<PythonModule, NUMBER>& modules) |
ref: https://github.com/xbmc/xbmc/blob/master/docs/CODE_GUIDELINES.md#86-methods
There was a problem hiding this comment.
This part is removed.
| @@ -58,9 +58,20 @@ class CPythonInvoker : public ILanguageInvoker | |||
| std::string m_sourceFile; | |||
| CCriticalSection m_critical; | |||
|
|
|||
| protected: | |||
| template<size_t NUMBER> | |||
There was a problem hiding this comment.
NUMBER isn't a great name, SIZE or COUNT would be more suitable.
There was a problem hiding this comment.
This part is removed.
|
|
||
| struct PythonModule | ||
| { | ||
| const char* name; |
There was a problem hiding this comment.
can we use std::string_view instead?
There was a problem hiding this comment.
PyImport_AppendInittab() and similar functions don't copy the module name. Using a non-dynamically allocated object like "const char *" is safer.
| { "xbmcplugin", PyInit_Module_xbmcplugin }, | ||
| { "xbmcaddon", PyInit_Module_xbmcaddon }, | ||
| { "xbmcvfs", PyInit_Module_xbmcvfs } | ||
| PythonModule{ "xbmcdrm", PyInit_Module_xbmcdrm }, |
There was a problem hiding this comment.
please don't use whitespace formatting, you can leave it to clang-format unless there really is a readability issue.
| PyObject* pyaddonid = PyUnicode_FromString(m_addon->ID().c_str()); | ||
| PyDict_SetItemString(moduleDictionary, "__xbmcaddonid__", pyaddonid); | ||
| PyDict_SetItemString(moduleDictionary, "__xbmcaddonid__", | ||
| PyObjectPtr(PyUnicode_FromString(m_addon->ID().c_str())).get()); |
There was a problem hiding this comment.
I feel like this could still be two lines, not a big deal though.
auto id = PyObjectPtr(PyUnicode_FromString(m_addon->ID().c_str()));
PyDict_SetItemString(moduleDictionary, "__xbmcaddonid__", id.get());
There was a problem hiding this comment.
This C++ syntax is not optimal, but it does the job.
|
I just realized that this could be simplified quite a bit by using Patchdiff --git a/xbmc/interfaces/python/AddonPythonInvoker.cpp b/xbmc/interfaces/python/AddonPythonInvoker.cpp
index b360c7d45d..7eef37c8ea 100644
--- a/xbmc/interfaces/python/AddonPythonInvoker.cpp
+++ b/xbmc/interfaces/python/AddonPythonInvoker.cpp
@@ -9,7 +9,6 @@
// python.h should always be included first before any other includes
#include "AddonPythonInvoker.h"
-#include <array>
#include <utility>
#include <Python.h>
@@ -88,14 +87,15 @@ using namespace PythonBindings;
namespace
{
// clang-format off
-const std::array<PythonModule, 6> PythonModules =
+const _inittab PythonModules[] =
{
- PythonModule{ "xbmcdrm", PyInit_Module_xbmcdrm },
- PythonModule{ "xbmcgui", PyInit_Module_xbmcgui },
- PythonModule{ "xbmc", PyInit_Module_xbmc },
- PythonModule{ "xbmcplugin", PyInit_Module_xbmcplugin },
- PythonModule{ "xbmcaddon", PyInit_Module_xbmcaddon },
- PythonModule{ "xbmcvfs", PyInit_Module_xbmcvfs }
+ { "xbmcdrm", PyInit_Module_xbmcdrm },
+ { "xbmcgui", PyInit_Module_xbmcgui },
+ { "xbmc", PyInit_Module_xbmc },
+ { "xbmcplugin", PyInit_Module_xbmcplugin },
+ { "xbmcaddon", PyInit_Module_xbmcaddon },
+ { "xbmcvfs", PyInit_Module_xbmcvfs },
+ { nullptr, nullptr }
};
// clang-format on
} // namespace
@@ -109,7 +109,8 @@ CAddonPythonInvoker::~CAddonPythonInvoker() = default;
void CAddonPythonInvoker::globalInitializeModules(void)
{
- initializeModules(PythonModules);
+ if (PyImport_ExtendInittab(const_cast<_inittab*>(PythonModules)))
+ CLog::Log(LOGWARNING, "CAddonPythonInvoker(): unable to extend inittab");
}
const char* CAddonPythonInvoker::getInitializationScript() const
diff --git a/xbmc/interfaces/python/PythonInvoker.cpp b/xbmc/interfaces/python/PythonInvoker.cpp
index 32b9089d8d..8da7becdcf 100644
--- a/xbmc/interfaces/python/PythonInvoker.cpp
+++ b/xbmc/interfaces/python/PythonInvoker.cpp
@@ -678,14 +678,6 @@ void CPythonInvoker::onError(const std::string& exceptionType /* = "" */,
}
}
-bool CPythonInvoker::initializeModule(const char* name, PythonModuleInitialization module)
-{
- if (module == NULL)
- return false;
-
- return !PyImport_AppendInittab(name, module);
-}
-
void CPythonInvoker::getAddonModuleDeps(const ADDON::AddonPtr& addon, std::set<std::string>& paths)
{
for (const auto& it : addon->GetDependencies())
diff --git a/xbmc/interfaces/python/PythonInvoker.h b/xbmc/interfaces/python/PythonInvoker.h
index 09b68c2f0e..e4ad95e8b4 100644
--- a/xbmc/interfaces/python/PythonInvoker.h
+++ b/xbmc/interfaces/python/PythonInvoker.h
@@ -18,7 +18,6 @@
#include <string>
#include <vector>
-struct PythonModule;
typedef struct _object PyObject;
class CPythonInvoker : public ILanguageInvoker
@@ -32,8 +31,6 @@ public:
bool IsStopping() const override { return m_stop || ILanguageInvoker::IsStopping(); }
- typedef PyObject* (*PythonModuleInitialization)();
-
protected:
// implementation of ILanguageInvoker
bool execute(const std::string& script, const std::vector<std::string>& arguments) override;
@@ -58,20 +55,7 @@ protected:
std::string m_sourceFile;
CCriticalSection m_critical;
-protected:
- template<size_t NUMBER>
- static void initializeModules(const std::array<PythonModule, NUMBER> modules)
- {
- for (const auto& module : modules)
- {
- if (!initializeModule(module.name, module.initialization))
- CLog::Log(LOGWARNING, "CPythonInvoker(): unable to initialize python module \"{}\"",
- module.name);
- }
- }
-
private:
- static bool initializeModule(const char* name, PythonModuleInitialization module);
void getAddonModuleDeps(const ADDON::AddonPtr& addon, std::set<std::string>& paths);
bool execute(const std::string& script, std::vector<std::wstring>& arguments);
FILE* PyFile_AsFileWithMode(PyObject* py_file, const char* mode);
@@ -94,9 +78,3 @@ private:
public:
typedef std::unique_ptr<PyObject, PyObjectDeleter> PyObjectPtr;
};
-
-struct PythonModule
-{
- const char* name;
- CPythonInvoker::PythonModuleInitialization initialization;
-};
diff --git a/xbmc/network/httprequesthandler/python/HTTPPythonWsgiInvoker.cpp b/xbmc/network/httprequesthandler/python/HTTPPythonWsgiInvoker.cpp
index 1dd3f2d33a..81880bcf97 100644
--- a/xbmc/network/httprequesthandler/python/HTTPPythonWsgiInvoker.cpp
+++ b/xbmc/network/httprequesthandler/python/HTTPPythonWsgiInvoker.cpp
@@ -19,7 +19,6 @@
#include "utils/StringUtils.h"
#include "utils/URIUtils.h"
-#include <array>
#include <utility>
#include <Python.h>
@@ -79,11 +78,12 @@ using namespace PythonBindings;
namespace
{
// clang-format off
-const std::array<PythonModule, 3> PythonModules =
+const _inittab PythonModules[] =
{
- PythonModule{ "xbmc", PyInit_Module_xbmc },
- PythonModule{ "xbmcaddon", PyInit_Module_xbmcaddon },
- PythonModule{ "xbmcwsgi", PyInit_Module_xbmcwsgi }
+ { "xbmc", PyInit_Module_xbmc },
+ { "xbmcaddon", PyInit_Module_xbmcaddon },
+ { "xbmcwsgi", PyInit_Module_xbmcwsgi },
+ { nullptr, nullptr }
};
// clang-format on
} // namespace
@@ -102,7 +102,8 @@ CHTTPPythonWsgiInvoker::~CHTTPPythonWsgiInvoker()
void CHTTPPythonWsgiInvoker::globalInitializeModules(void)
{
- initializeModules(PythonModules);
+ if (PyImport_ExtendInittab(const_cast<_inittab*>(PythonModules)))
+ CLog::Log(LOGWARNING, "CHTTPPythonWsgiInvoker(): unable to extend inittab");
}
HTTPPythonRequest* CHTTPPythonWsgiInvoker::GetRequest() |
repojohnray
force-pushed
the
cpythoninvokercodecleanup
branch
from
83e983c
to
58498ea
Compare
|
@neo1973, the last commit is updated with your patch. Kodi is working flawlessly. |
repojohnray
force-pushed
the
cpythoninvokercodecleanup
branch
from
58498ea
to
4bf9de8
Compare
|
This last change updates the comment. |
Description
This commit fixes the memory leaks and updates initializeModule(). The function initializeModule() was allocating the python objects which were immediately leaked. The proper python initialization of these modules is done by PyImport_AppendInittab().
Motivation and context
The goal is to clean up some issues detected by the sanitizer.
How has this been tested?
The option "-fsanitize=leak" is required and should be passed to the compiler and the linker. For a fully sanitized executable, you could use the following options: "-fsanitize=bounds -fsanitize=address -fsanitize=undefined -fsanitize=unreachable -fsanitize=bool -fsanitize=enum -fsanitize=leak -fsanitize-recover=address"
An example below, or you could use the cmake option: -DECM_ENABLE_SANITIZERS
cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS_DEBUG="-ggdb -O2 -fno-omit-frame-pointer -fsanitize=leak" -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=leak" -DCMAKE_SHARED_LINKER_FLAGS="-fsanitize=leak"
The gcc sanitizer operates properly with a debug version of Python-3.11 compiled with the following options: '--without-pymalloc --with-pydebug --with-valgrind'. As a reminder with Python-3.8 the same options were not enough to generate a "sanitized" compatible python library.
What is the effect on users?
Screenshots (if appropriate):
Types of change
Checklist: