Skip to content

Commit

Permalink
Alter ScopedPythonGIL to follow the std::lock_guard pattern
Browse files Browse the repository at this point in the history 
This allows a PythonGIL object to be stored in order to workaround
a quirk in the Python GILStateAPI
  • Loading branch information
@martyngigg
martyngigg committed Sep 3, 2017
1 parent e19b978 commit 26335d6
Show file tree
Hide file tree
Showing 7 changed files with 97 additions and 50 deletions.
6 changes: 4 additions & 2 deletions MantidPlot/src/Mantid/MantidUI.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,8 @@ void MantidUI::shutdown() {

// If any python objects need to be cleared away then the GIL needs to be
// held.
ScopedPythonGIL gil;
PythonGIL gil;
ScopedPythonGIL lock(gil);
// Relevant notifications are connected to signals that will close all
// dependent windows
Mantid::API::FrameworkManager::Instance().shutdown();
Expand Down Expand Up @@ -2216,7 +2217,8 @@ void MantidUI::clearAllMemory(const bool prompt) {
// If any python objects need to be cleared away then the GIL needs to be
// held. This doesn't feel like
// it is in the right place but it will do no harm
ScopedPythonGIL gil;
PythonGIL gil;
ScopedPythonGIL lock(gil);
// Relevant notifications are connected to signals that will close all
// dependent windows
Mantid::API::FrameworkManager::Instance().clear();
Expand Down
72 changes: 39 additions & 33 deletions MantidPlot/src/PythonScript.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,18 +78,18 @@ static const QString MSG_STARTED = "Script execution started.";
*/
PythonScript::PythonScript(PythonScripting *env, const QString &name,
const InteractionType interact, QObject *context)
: Script(env, name, interact, context), m_pythonEnv(env), localDict(NULL),
: Script(env, name, interact, context), m_interp(env), localDict(NULL),
stdoutSave(NULL), stderrSave(NULL), m_codeFileObject(NULL),
m_threadID(-1), isFunction(false), m_isInitialized(false),
m_pathHolder(name), m_workspaceHandles() {
m_pathHolder(name, *this), m_workspaceHandles() {
initialize(name, context);
}

/**
* Destructor
*/
PythonScript::~PythonScript() {
ScopedPythonGIL lock;
ScopedPythonGIL lock(gil());
this->abort();
observeAdd(false);
observeAfterReplace(false);
Expand Down Expand Up @@ -142,7 +142,7 @@ PyObject *PythonScript::createSipInstanceFromMe() {
*/
bool PythonScript::compilesToCompleteStatement(const QString &code) const {
bool result(false);
ScopedPythonGIL gil;
ScopedPythonGIL lock(gil());
PyObject *compiledCode = Py_CompileString(code.toAscii(), "", Py_file_input);
if (PyObject *exception = PyErr_Occurred()) {
// Certain exceptions still mean the code is complete
Expand Down Expand Up @@ -186,16 +186,16 @@ void PythonScript::sendLineChangeSignal(int lineNo, bool error) {
* Create a list autocomplete keywords
*/
void PythonScript::generateAutoCompleteList() {
ScopedPythonGIL gil;
ScopedPythonGIL lock(gil());
PyObject *keywords = PyObject_CallFunctionObjArgs(
PyDict_GetItemString(m_pythonEnv->globalDict(),
PyDict_GetItemString(m_interp->globalDict(),
"_ScopeInspector_GetFunctionAttributes"),
localDict, NULL);
if (PyErr_Occurred() || !keywords) {
PyErr_Print();
return;
}
QStringList keywordList = pythonEnv()->toStringList(keywords);
QStringList keywordList = interp()->toStringList(keywords);
Py_DECREF(keywords);
emit autoCompleteListGenerated(keywordList);
}
Expand All @@ -206,7 +206,7 @@ void PythonScript::generateAutoCompleteList() {
*/
void PythonScript::emit_error() {
// gil is necessary so other things don't continue
ScopedPythonGIL gil;
ScopedPythonGIL lock(gil());

// return early if nothing happened
if (!PyErr_Occurred()) {
Expand Down Expand Up @@ -286,12 +286,12 @@ void PythonScript::emit_error() {
* @param syntaxError An instance of PyExc_SyntaxError
*/
QString PythonScript::constructSyntaxErrorStr(PyObject *syntaxError) {
QString exceptionAsStr = m_pythonEnv->toString(syntaxError);
QString exceptionAsStr = m_interp->toString(syntaxError);
exceptionAsStr = exceptionAsStr.section("(", 0, 0).trimmed();
const QString filename = m_pythonEnv->toString(
PyObject_GetAttrString(syntaxError, "filename"), true);
const QString filename =
m_interp->toString(PyObject_GetAttrString(syntaxError, "filename"), true);
int lineno = static_cast<int>(
m_pythonEnv->toLong(PyObject_GetAttrString(syntaxError, "lineno")));
m_interp->toLong(PyObject_GetAttrString(syntaxError, "lineno")));
#if PY_VERSION_HEX < 0x02070000
// Syntax errors generated by earlier versions seem to have a line number off
// by 1
Expand All @@ -303,9 +303,9 @@ QString PythonScript::constructSyntaxErrorStr(PyObject *syntaxError) {
// using a ^ character
PyObject *textObject = PyObject_GetAttrString(syntaxError, "text");
if (textObject != Py_None) {
QString text = m_pythonEnv->toString(textObject, true).trimmed();
QString text = m_interp->toString(textObject, true).trimmed();
int offset = static_cast<int>(
m_pythonEnv->toLong(PyObject_GetAttrString(syntaxError, "offset")));
m_interp->toLong(PyObject_GetAttrString(syntaxError, "offset")));
QString offsetMarker = QString(offset - 1, ' ') + "^";
msg = "File \"%1\", line %2\n"
" %3\n"
Expand Down Expand Up @@ -357,17 +357,17 @@ void PythonScript::tracebackToMsg(QTextStream &msgStream,

bool PythonScript::setQObject(QObject *val, const char *name) {
if (localDict) {
return pythonEnv()->setQObject(val, name, localDict);
return interp()->setQObject(val, name, localDict);
} else
return false;
}

bool PythonScript::setInt(int val, const char *name) {
return pythonEnv()->setInt(val, name, localDict);
return interp()->setInt(val, name, localDict);
}

bool PythonScript::setDouble(double val, const char *name) {
return pythonEnv()->setDouble(val, name, localDict);
return interp()->setDouble(val, name, localDict);
}

void PythonScript::setContext(QObject *context) {
Expand All @@ -380,7 +380,7 @@ void PythonScript::setContext(QObject *context) {
* the dictionary context back to the default set
*/
void PythonScript::clearLocals() {
ScopedPythonGIL pythonLock;
ScopedPythonGIL lock(gil());

PyObject *mainModule = PyImport_AddModule("__main__");
PyObject *cleanLocals = PyDict_Copy(PyModule_GetDict(mainModule));
Expand All @@ -397,6 +397,12 @@ void PythonScript::clearLocals() {
localDict = cleanLocals;
}

/**
* @brief PythonScript::gil
* @return A reference to the global lock
*/
PythonGIL &PythonScript::gil() const { return interp()->gil(); }

/**
* Sets the context for the script and if name points to a file then
* sets the __file__ variable
Expand All @@ -406,7 +412,7 @@ void PythonScript::clearLocals() {
void PythonScript::initialize(const QString &name, QObject *context) {
clearLocals(); // holds and releases GIL

ScopedPythonGIL pythonlock;
ScopedPythonGIL lock(gil());
PythonScript::setIdentifier(name);
setContext(context);

Expand All @@ -426,12 +432,12 @@ void PythonScript::initialize(const QString &name, QObject *context) {
void PythonScript::beginStdoutRedirect() {
if (!redirectStdOut())
return;
stdoutSave = PyDict_GetItemString(pythonEnv()->sysDict(), "stdout");
stdoutSave = PyDict_GetItemString(interp()->sysDict(), "stdout");
Py_XINCREF(stdoutSave);
stderrSave = PyDict_GetItemString(pythonEnv()->sysDict(), "stderr");
stderrSave = PyDict_GetItemString(interp()->sysDict(), "stderr");
Py_XINCREF(stderrSave);
pythonEnv()->setQObject(this, "stdout", pythonEnv()->sysDict());
pythonEnv()->setQObject(this, "stderr", pythonEnv()->sysDict());
interp()->setQObject(this, "stdout", interp()->sysDict());
interp()->setQObject(this, "stderr", interp()->sysDict());
}

/**
Expand All @@ -442,9 +448,9 @@ void PythonScript::endStdoutRedirect() {
if (!redirectStdOut())
return;

PyDict_SetItemString(pythonEnv()->sysDict(), "stdout", stdoutSave);
PyDict_SetItemString(interp()->sysDict(), "stdout", stdoutSave);
Py_XDECREF(stdoutSave);
PyDict_SetItemString(pythonEnv()->sysDict(), "stderr", stderrSave);
PyDict_SetItemString(interp()->sysDict(), "stderr", stderrSave);
Py_XDECREF(stderrSave);
}

Expand All @@ -464,7 +470,7 @@ bool PythonScript::compileImpl() {
* @return
*/
QVariant PythonScript::evaluateImpl() {
ScopedPythonGIL gil;
ScopedPythonGIL lock(gil());
PyObject *compiledCode = this->compileToByteCode(true);
if (!compiledCode) {
return QVariant("");
Expand Down Expand Up @@ -590,8 +596,8 @@ void PythonScript::abortImpl() {
// hasn't implemented cancel() checking so that when control returns the
// Python the
// interrupt should be picked up.
ScopedPythonGIL lock;
m_pythonEnv->raiseAsyncException(m_threadID, PyExc_KeyboardInterrupt);
ScopedPythonGIL lock(gil());
m_interp->raiseAsyncException(m_threadID, PyExc_KeyboardInterrupt);
PyObject *curAlg =
PyObject_CallFunction(m_algorithmInThread, STR_LITERAL("l"), m_threadID);
if (curAlg && curAlg != Py_None) {
Expand All @@ -605,15 +611,15 @@ void PythonScript::abortImpl() {
* @return A long int giving a unique ID for the thread
*/
long PythonScript::getThreadID() {
ScopedPythonGIL lock;
ScopedPythonGIL lock(gil());
return PyThreadState_Get()->thread_id;
}

/// Performs the call to Python
bool PythonScript::executeString() {
emit started(MSG_STARTED);
bool success(false);
ScopedPythonGIL gil;
ScopedPythonGIL lock(gil());

PyObject *compiledCode = compileToByteCode(false);
PyObject *result(NULL);
Expand All @@ -629,7 +635,7 @@ bool PythonScript::executeString() {
// call Algorithm::cancel to make sure we capture it. The doubling
// can leave an interrupt in the pipeline so we clear it was we've
// got the error info out
m_pythonEnv->raiseAsyncException(m_threadID, NULL);
m_interp->raiseAsyncException(m_threadID, NULL);
} else {
emit finished(MSG_FINISHED);
success = true;
Expand Down Expand Up @@ -706,7 +712,7 @@ PyObject *PythonScript::compileToByteCode(bool for_eval) {
// __builtins__.
PyDict_SetItemString(
localDict, "__builtins__",
PyDict_GetItemString(pythonEnv()->globalDict(), "__builtins__"));
PyDict_GetItemString(interp()->globalDict(), "__builtins__"));
PyObject *ret = PyRun_String(
"def col(c,*arg):\n"
"\ttry: return self.cell(c,arg[0])\n"
Expand All @@ -728,7 +734,7 @@ PyObject *PythonScript::compileToByteCode(bool for_eval) {
// __builtins__.
PyDict_SetItemString(
localDict, "__builtins__",
PyDict_GetItemString(pythonEnv()->globalDict(), "__builtins__"));
PyDict_GetItemString(interp()->globalDict(), "__builtins__"));
PyObject *ret =
PyRun_String("def cell(*arg):\n"
"\ttry: return self.cell(arg[0],arg[1])\n"
Expand Down
39 changes: 32 additions & 7 deletions MantidPlot/src/PythonScript.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@ class PythonScript : public Script, MantidQt::API::WorkspaceObserver {
/// Helper class to ensure the sys.path variable is updated correctly
struct PythonPathHolder {
/// Update the path with the given entry
explicit PythonPathHolder(const QString &entry) : m_path(entry) {
explicit PythonPathHolder(const QString &entry, const PythonScript &script)
: m_path(entry), m_script(script) {
const QFileInfo filePath(m_path);
if (filePath.exists()) {
QDir directory = filePath.absoluteDir();
Expand All @@ -125,14 +126,14 @@ class PythonScript : public Script, MantidQt::API::WorkspaceObserver {
}

void appendPath(const QString &path) {
ScopedPythonGIL pythonLock;
ScopedPythonGIL lock(m_script.gil());
QString code = "if r'%1' not in sys.path:\n"
" sys.path.append(r'%1')";
code = code.arg(path);
PyRun_SimpleString(code.toAscii().constData());
}
void removePath(const QString &path) {
ScopedPythonGIL pythonLock;
ScopedPythonGIL lock(m_script.gil());
QString code = "if r'%1' in sys.path:\n"
" sys.path.remove(r'%1')";
code = code.arg(path);
Expand All @@ -141,15 +142,39 @@ class PythonScript : public Script, MantidQt::API::WorkspaceObserver {

private:
QString m_path;
const PythonScript &m_script;
};

inline PythonScripting *pythonEnv() const { return m_pythonEnv; }
inline PythonScripting *interp() const { return m_interp; }
PythonGIL &gil() const;
void initialize(const QString &name, QObject *context);
void beginStdoutRedirect();
void endStdoutRedirect();

// --------------------------- Script compilation/execution
// -----------------------------------
// Pre and post-processing options for recursive calls.
// The Python GILState api is supposed to be able to ensure
// that the current thread is ready to execute Python code
// regardless of the current interpreter thread state.
// However, we have a case with the ApplicationWindow::runPythonScript
// method where a script can be sent to that method to be run with
// async=false yet the script itself contains a runPythonScript(async=True)
// call. This causes recursion into the PythonGIL object where
// the initial GIL has not been released and a deadlock ensues.
// An example script sent to runPythonScript(async=False) would be
//
// import mantidplot
// for i in range(10):
// mantidplot.runPythonScript('test=CreateSampleWorkspace()', True)
//
// To circumvent this we must release the GIL on the main thread
// before starting the async thread and then reaquire it when that thread
// has finished and the main thread must keep executing. These methods
// are used for this purpose and are NOT used by the general executeAsync
// methods where the GILState API functions can cope and there is no
// recursion.
virtual void recursiveAsyncSetup() override;
virtual void recursiveAsyncTeardown() override;

/// Compile the code, returning true if it was successful, false otherwise
bool compileImpl() override;
/// Evaluate the current code and return a result as a QVariant
Expand Down Expand Up @@ -191,7 +216,7 @@ class PythonScript : public Script, MantidQt::API::WorkspaceObserver {
/// Send out an error and clear it from python.
void emit_error();

PythonScripting *m_pythonEnv;
PythonScripting *m_interp;
PyObject *localDict, *stdoutSave, *stderrSave;
PyObject *m_codeFileObject;
long m_threadID; ///< Python thread id
Expand Down
5 changes: 2 additions & 3 deletions MantidPlot/src/PythonScripting.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,15 @@ ScriptingEnv *PythonScripting::constructor(ApplicationWindow *parent) {
/** Constructor */
PythonScripting::PythonScripting(ApplicationWindow *parent)
: ScriptingEnv(parent, "Python"), m_globals(NULL), m_math(NULL),
m_sys(NULL), m_mainThreadState(NULL) {}
m_sys(NULL), m_mainThreadState(NULL), m_gil() {}

PythonScripting::~PythonScripting() {}

/**
* @param args A list of strings that denoting command line arguments
*/
void PythonScripting::setSysArgs(const QStringList &args) {
ScopedPythonGIL gil;

ScopedPythonGIL lock(gil());
PyObject *argv = toPyList(args);
if (argv && m_sys) {
PyDict_SetItemString(m_sys, "argv", argv);
Expand Down
4 changes: 4 additions & 0 deletions MantidPlot/src/PythonScripting.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,8 @@ class PythonScripting : public ScriptingEnv {
PyObject *globalDict() { return m_globals; }
/// Return the sys dictionary for this environment
PyObject *sysDict() { return m_sys; }
/// Return a reference to the global lock
PythonGIL &gil() { return m_gil; }

private:
/// Constructor
Expand All @@ -139,6 +141,8 @@ class PythonScripting : public ScriptingEnv {
PyObject *m_sys;
/// Pointer to the main threads state
PyThreadState *m_mainThreadState;
/// Wrap's acquisition of the GIL
PythonGIL m_gil;
};

#endif
Loading

0 comments on commit 26335d6

Please sign in to comment.