Skip to content

Commit 004e691

Browse files
brudersteinbruderstein
committed
GILLock / GILRelease check if they have the GIL with API
In the case of 3 threads fighting over the GIL, it was possible for a deadlock to occur, the thread 2 stole the GIL from thread 1, and thread 3 stole from thread 2. The API call to check if you have the GIL was added as a helper function in Python 3.4.0 (issue #17522). The function is taken from there. A backport would be trivial, but that would mean we could only use the packaged python27.dll, and not the official one.
1 parent de77337 commit 004e691

9 files changed

Lines changed: 687 additions & 773 deletions

‎PythonScript/src/CallbackExecArgs.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ namespace NppPythonScript
88

99
CallbackExecArgs::~CallbackExecArgs()
1010
{
11-
GILLock gilLock = GILManager::getGIL();
11+
GILLock gilLock;
1212
delete m_callbacks;
1313
if (NULL != m_params)
1414
{

‎PythonScript/src/CreateWrapper.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ def Contains(s,sub):
8080
return s.find(sub) != -1
8181

8282
def releaseGIL():
83-
return "\tGILRelease gilRelease = GILManager::releaseGIL();\n"
83+
return "\tGILRelease gilRelease;\n"
8484

8585
def reacquireGIL():
8686
return "\tgilRelease.reacquire();\n"

‎PythonScript/src/GILManager.cpp‎

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,19 +3,16 @@
33

44
namespace NppPythonScript
55
{
6-
GILManager* GILManager::s_gilManagerInstance = NULL;
6+
// GILManager* GILManager::s_gilManagerInstance = NULL;
77

8-
GILLock::GILLock(GILManager* manager, bool takeLock)
9-
: m_manager(manager),
10-
m_hasLock(takeLock)
8+
GILLock::GILLock()
9+
: m_hasLock(false == doIHaveTheGIL())
1110
{
12-
if (takeLock)
11+
if (m_hasLock)
1312
{
1413
DEBUG_TRACE(L"Acquiring GIL...\n");
1514
m_state = PyGILState_Ensure();
1615
DEBUG_TRACE(L"GIL Acquired.\n");
17-
m_manager->setThreadWithGIL();
18-
DEBUG_TRACE(L"ThreadID set after GIL acquired\n");
1916
}
2017
}
2118

@@ -31,23 +28,25 @@ namespace NppPythonScript
3128
if (m_hasLock)
3229
{
3330
DEBUG_TRACE(L"Releasing GIL...\n");
34-
m_manager->release();
3531
PyGILState_Release(m_state);
3632
DEBUG_TRACE(L"GIL Released.\n");
3733
}
3834
}
3935

4036

41-
GILRelease::GILRelease(GILManager* manager, bool releaseLock)
42-
: m_manager(manager),
43-
m_lockReleased(releaseLock)
37+
GILRelease::GILRelease()
38+
: m_lockReleased(doIHaveTheGIL())
4439
{
45-
if (releaseLock)
40+
if (m_lockReleased)
4641
{
4742
DEBUG_TRACE(L"Temp GIL Release requested\n");
48-
m_manager->release();
4943
m_threadState = PyEval_SaveThread();
5044
}
45+
else
46+
{
47+
DEBUG_TRACE(L"Temp GIL Release ignored- we don't have the GIL\n");
48+
}
49+
5150
}
5251

5352
GILRelease::~GILRelease()
@@ -57,8 +56,6 @@ namespace NppPythonScript
5756
DEBUG_TRACE(L"Re-acquiring GIL after temporary release\n");
5857
PyEval_RestoreThread(m_threadState);
5958
DEBUG_TRACE(L"GIL reacquired after temporary release\n");
60-
m_manager->setThreadWithGIL();
61-
DEBUG_TRACE(L"Manager updated with new thread ID\n");
6259
}
6360
}
6461

@@ -70,7 +67,6 @@ namespace NppPythonScript
7067
PyEval_RestoreThread(m_threadState);
7168
DEBUG_TRACE(L"GIL Reacquired after temporary release (in reacquire())\n");
7269
m_lockReleased = false;
73-
m_manager->setThreadWithGIL();
7470
}
7571
}
7672
}

‎PythonScript/src/GILManager.h‎

Lines changed: 46 additions & 127 deletions
Original file line numberDiff line numberDiff line change
@@ -7,47 +7,71 @@ namespace NppPythonScript
77
{
88
/*
99
10-
This whole schenanigans seems necessary, as you don't appear to be able to call PyGILState_Ensure() from a thread that already has the GIL.
11-
It just locks up, and digging into the code, it's waiting on a lock that the thread already has.
10+
This whole schenanigans is necessary, as you can't call PyGILState_Ensure() from a thread that already has the GIL.
1211
13-
For callbacks, it's impossible to know if the callback was generated from "within" python or not.
14-
For instance: you have a callback on scintilla SCN_MODIFIED. If from a script you do editor.write('...'), you've got the GIL still when the callback comes.
15-
When the user makes a change, it calls the callback, but this time you *don't* have the GIL, so you need to get it.
12+
For the ScintillaWrapper (specifically, but not exclusively), it's tricky to impossible to tell if we need to release the GIL. If we're running on the
13+
Python worker thread, and we make a call to Scintilla, that triggers a callback, we need to give it up (in order that the callback can run). The callback
14+
identification must be performed with the GIL, so it would block if we didn't give it up. (There's an optimisation that if no callbacks are registered, then it
15+
doesn't check, and doesn't acquire the GIL)
16+
17+
However, if we're being called from the replace function, and therefore the main thread, we probably don't even have the GIL to give up.
1618
1719
With the different threads running the different callbacks, and python code running on a thread (*generally* - see the editor.replace() suite)
1820
it has become impossible to manage manually (read: the Wrong Way To Do It(tm)). These objects manage the GIL, and provide a easy, safe way to ensure
1921
you have the GIL when you need it, and give it up correctly whatever happens in the mean time.
20-
*/
2122
22-
class GILManager;
23+
The doIHaveTheGIL() function is taken from Python 3.4.0, Issue #17522. An extra workaround is included to make it work when Py_DEBUG is defined.
24+
- see note below where PyThreadState_GET is redefined.
25+
*/
2326

2427

25-
/* GILLock is the generic class given back when asking for the lock
26-
* If the current thread already has the lock, then you'll get an instance of
27-
* this class, that does nothing.
28-
* If however, you didn't already have the lock, you'll an instance of OwnedGILLock, which
29-
* gives the lock back upon destruction.
28+
#ifdef Py_DEBUG
29+
/* For Py_DEBUG, PyThreadState_GET() is redefined to PyThreadState_Get(), which checks that the current ThreadState is not null
30+
* We "undo" that define here, as we need to get the current threadstate without checking if it's not set
3031
*/
32+
#undef PyThreadState_GET
33+
#define PyThreadState_GET() (_PyThreadState_Current)
34+
#endif
3135

32-
36+
class GILHandler
37+
{
38+
protected:
39+
static bool doIHaveTheGIL()
40+
{
3341

34-
35-
class GILLock
42+
PyThreadState* thisThreadState = PyGILState_GetThisThreadState();
43+
return (thisThreadState && thisThreadState == PyThreadState_GET());
44+
}
45+
46+
};
47+
48+
49+
/* GILLock holds the GIL whilst it exists. If you already have the GIL, it will do nothing.
50+
* If however, you didn't already have the lock, it will obtain the lock on construction,
51+
* and release it back upon destruction.
52+
*/
53+
class GILLock : public GILHandler
3654
{
3755
public:
38-
GILLock(GILManager* manager, bool takeLock);
56+
GILLock();
3957
virtual ~GILLock();
4058

4159
private:
42-
GILManager* m_manager;
60+
GILLock(const GILLock&); /* disable copying */
61+
GILLock& operator = (const GILLock&); /* disable assignment */
4362
PyGILState_STATE m_state;
4463
bool m_hasLock;
4564
};
4665

47-
class GILRelease
66+
67+
/* GILRelease will release the GIL if you have it for as long as the object exists.
68+
* On destruction it will reacquire the lock. You can reacquire the GIL sooner than
69+
* destruction by calling reacquire(). This makes the destructor a no-op.
70+
*/
71+
class GILRelease : public GILHandler
4872
{
4973
public:
50-
GILRelease(GILManager *manager, bool releaseLock);
74+
GILRelease();
5175
virtual ~GILRelease();
5276

5377
/** Reacquire the GIL after releasing it
@@ -58,119 +82,14 @@ namespace NppPythonScript
5882
void reacquire();
5983

6084
private:
61-
GILManager* m_manager;
85+
GILRelease(const GILRelease&);
86+
GILRelease& operator = (const GILRelease&); /* disable assignment */
6287
PyThreadState *m_threadState;
6388
bool m_lockReleased;
6489
};
6590

66-
class GILManager
67-
{
68-
public:
69-
static GILLock getGIL()
70-
{
71-
if (NULL == s_gilManagerInstance)
72-
{
73-
s_gilManagerInstance = new GILManager();
74-
}
75-
return s_gilManagerInstance->getGILImpl();
76-
}
7791

78-
static GILRelease releaseGIL()
79-
{
80-
if (NULL == s_gilManagerInstance)
81-
{
82-
s_gilManagerInstance = new GILManager();
83-
}
84-
return s_gilManagerInstance->releaseGILImpl();
85-
}
86-
87-
void release()
88-
{
89-
::EnterCriticalSection(&m_criticalSection);
90-
m_currentThreadWithGIL = m_originalThreadWithGIL;
91-
m_originalThreadWithGIL = 0;
92-
DEBUG_TRACE(L"current thread with GIL now 0\n");
93-
::LeaveCriticalSection(&m_criticalSection);
94-
}
95-
96-
void setThreadWithGIL()
97-
{
98-
::EnterCriticalSection(&m_criticalSection);
99-
m_originalThreadWithGIL = m_currentThreadWithGIL;
100-
m_currentThreadWithGIL = ::GetCurrentThreadId();
101-
DEBUG_TRACE(L"current thread with GIL now [threadid]\n");
102-
::LeaveCriticalSection(&m_criticalSection);
103-
}
104-
105-
private:
106-
107-
// Disallow public construction
108-
GILManager() :
109-
m_currentThreadWithGIL(0),
110-
m_originalThreadWithGIL(0),
111-
m_tempCorruptionCheck(42)
112-
113-
{
114-
InitializeCriticalSection(&m_criticalSection);
115-
}
116-
117-
// Disallow copying
118-
GILManager(const GILManager& );
119-
120-
GILLock getGILImpl()
121-
{
122-
bool needLock;
123-
DWORD currentThread = ::GetCurrentThreadId();
124-
125-
::EnterCriticalSection(&m_criticalSection);
126-
needLock = (currentThread != m_currentThreadWithGIL);
127-
::LeaveCriticalSection(&m_criticalSection);
128-
129-
if (needLock)
130-
{
131-
// Someone else (or no-one) has the GIL, so we want to acquire it, and give it back when the object clears up
132-
DEBUG_TRACE(L"Thread does not have GIL, requesting\n");
133-
134-
}
135-
else
136-
{
137-
// The current thread already has the GIL, so just give an empty object back
138-
DEBUG_TRACE(L"Thread already has GIL - ignoring request\n");
139-
}
140-
return GILLock(this, needLock);
141-
}
142-
143-
GILRelease releaseGILImpl()
144-
{
145-
bool hasLock;
146-
147-
DWORD currentThread = ::GetCurrentThreadId();
148-
::EnterCriticalSection(&m_criticalSection);
149-
hasLock = (currentThread == m_currentThreadWithGIL);
150-
::LeaveCriticalSection(&m_criticalSection);
151-
152-
if (hasLock)
153-
{
154-
DEBUG_TRACE(L"Thread has GIL - will be released\n");
155-
}
156-
else
157-
{
158-
DEBUG_TRACE(L"Thread does not have GIL - ignoring release request\n");
159-
//hasLock = true;
160-
}
161-
162-
return GILRelease(this, hasLock);
163-
164-
}
165-
166-
167-
DWORD m_currentThreadWithGIL;
168-
DWORD m_originalThreadWithGIL;
169-
DWORD m_tempCorruptionCheck;
170-
CRITICAL_SECTION m_criticalSection;
171-
static GILManager *s_gilManagerInstance;
172-
173-
};
92+
17493

17594
}
17695
#endif // GILMANAGER_20140215_H

0 commit comments

Comments
 (0)