Skip to content
This repository

FIX: GilSafeSingleLock is causing UI freeze when it cannot get the GIL back... #1204

Closed
wants to merge 1 commit into from

4 participants

Chris Browet jmarshallnz Jim Carroll Garrett Brown
Chris Browet
Collaborator
koying commented

... while having already locked the graphic context.

This happens (for me), for instance, during lengthy mysql operations in python while another script tries a simple "window.setProperty".

Removing it and using the separate ad-hoc locking mechanisms. Reference: Pull Request #797

Chris Browet koying FIX: GilSafeSingleLock is causing UI freeze when it cannot get the GI…
…L back while having already locked the graphic context. Removing it.
1ad325a
Garrett Brown

cosmetic (two little spaces thought they could come along for the ride :))

jmarshallnz

Above this you call PyThreadState_Get() when setting the callback. What implications (if any) does this have while you're not holding the GIL?

Collaborator

ACK, You need the GIL for PyThreadState_Get.

jmarshallnz

above this you're accessing python functions without the GIL?

Collaborator

ACK

jmarshallnz
Owner

@jimfcarroll a review would be good - a few places we're calling python routines while in the scope of CPyThreadState that I'm not sure about.

Chris Browet
Collaborator
koying commented

Actually, looks like all CGUIWindowManager functions are already locking the graphic context, so I wonder if it is even necessary to recurse lock it in the windows.cpp functions... Will test...

Jim Carroll
Collaborator

My concern here is whether or not you considered that calls made from these methods assume the GIL is held and not just what's visible here. For example, you are now calling "new CGUIPythonWindowXMLDialog" without holding the GIL. Did you check that all possible call stacks resulting from all calls where you no longer are holding the GIL nowhere assume you were holding the GIL?

This is why I did it the way I did in the first place. Not that I think it was the right answer (The right answer is in my PR).

Chris Browet
Collaborator
koying commented

I checked as far as possible that called functions were not calling python.

But my problem is actually related to the graphic context lock, not the GIL. As far as my problem is concerned, the functions can have the GIL as long as the UI is not locked while we are waiting to obtain it, as was done with GilSafeSingleLock.

So, if someone could confirm the "CSingleLock lock(g_graphicsContext);"'s are not needed because the locking is properly done in the xbmc window functions, that would solve the original problem, the windows.cpp could keep the GIL and no "CPyThreadState" would even be necessary.

Chris Browet koying closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Jul 22, 2012
Chris Browet koying FIX: GilSafeSingleLock is causing UI freeze when it cannot get the GI…
…L back while having already locked the graphic context. Removing it.
1ad325a
This page is out of date. Refresh to see the latest.

Showing 1 changed file with 64 additions and 24 deletions. Show diff stats Hide diff stats

  1. +64 24 xbmc/interfaces/python/xbmcmodule/window.cpp
88 xbmc/interfaces/python/xbmcmodule/window.cpp
@@ -41,17 +41,7 @@ using namespace std;
41 41 #define ACTIVE_WINDOW g_windowManager.GetActiveWindow()
42 42
43 43
44   -/**
45   - * A CSingleLock that will relinquish the GIL during the time
46   - * it takes to obtain the CriticalSection
47   - */
48   -class GilSafeSingleLock : public CPyThreadState, public CSingleLock
49   -{
50   -public:
51   - GilSafeSingleLock(const CCriticalSection& critSec) : CPyThreadState(true), CSingleLock(critSec) { CPyThreadState::Restore(); }
52   -};
53   -
54   -#ifdef __cplusplus
  44 +#ifdef __cplusplus
55 45 extern "C" {
56 46 #endif
57 47
@@ -62,8 +52,9 @@ namespace PYXBMC
62 52 // used by Dialog to to create a new dialogWindow
63 53 bool Window_CreateNewWindow(Window* pWindow, bool bAsDialog)
64 54 {
65   - GilSafeSingleLock lock(g_graphicsContext);
66   -
  55 + CPyThreadState state;
  56 +
  57 + CSingleLock lock(g_graphicsContext);
67 58 if (pWindow->iWindowId != -1)
68 59 {
69 60 // user specified window id, use this one if it exists
@@ -71,6 +62,8 @@ namespace PYXBMC
71 62 pWindow->pWindow = g_windowManager.GetWindow(pWindow->iWindowId);
72 63 if (!pWindow->pWindow)
73 64 {
  65 + lock.Leave();
  66 + state.Restore();
74 67 PyErr_SetString(PyExc_ValueError, "Window id does not exist");
75 68 return false;
76 69 }
@@ -84,6 +77,8 @@ namespace PYXBMC
84 77 // if window 13099 is in use it means python can't create more windows
85 78 if (g_windowManager.GetWindow(WINDOW_PYTHON_END))
86 79 {
  80 + lock.Leave();
  81 + state.Restore();
87 82 PyErr_SetString(PyExc_Exception, "maximum number of windows reached");
88 83 return false;
89 84 }
@@ -111,6 +106,9 @@ namespace PYXBMC
111 106
112 107 g_windowManager.Add(pWindow->pWindow);
113 108 }
  109 + lock.Leave();
  110 + state.Restore();
  111 +
114 112 pWindow->iOldWindowId = 0;
115 113 pWindow->bModal = false;
116 114 pWindow->iCurrentControlId = 3000;
@@ -139,13 +137,16 @@ namespace PYXBMC
139 137 }
140 138
141 139 // lock xbmc GUI before accessing data from it
142   - GilSafeSingleLock lock(g_graphicsContext);
  140 + CPyThreadState state;
  141 + CSingleLock lock(g_graphicsContext);
143 142
144 143 // check if control exists
145 144 CGUIControl* pGUIControl = (CGUIControl*)self->pWindow->GetControl(iControlId);
146 145 if (!pGUIControl)
147 146 {
148 147 // control does not exist.
  148 + lock.Leave();
  149 + state.Restore();
149 150 CStdString error;
150 151 error.Format("Non-Existent Control %d",iControlId);
151 152 PyErr_SetString(PyExc_TypeError, error.c_str());
@@ -282,6 +283,9 @@ namespace PYXBMC
282 283 break;
283 284 }
284 285
  286 + lock.Leave();
  287 + state.Restore();
  288 +
285 289 if (!pControl)
286 290 {
287 291 // throw an exeption
@@ -342,7 +346,9 @@ namespace PYXBMC
342 346
343 347 void Window_Dealloc(Window* self)
344 348 {
345   - GilSafeSingleLock lock(g_graphicsContext);
  349 + CPyThreadState state;
  350 +
  351 + CSingleLock lock(g_graphicsContext);
346 352 if (self->bIsPythonWindow)
347 353 {
348 354 // first change to an existing window
@@ -399,6 +405,8 @@ namespace PYXBMC
399 405 }
400 406
401 407 lock.Leave();
  408 + state.Restore();
  409 +
402 410 self->vecControls.clear();
403 411 self->vecControls.~vector();
404 412 self->sFallBackPath.~string();
@@ -632,9 +640,13 @@ namespace PYXBMC
632 640 pControl->iParentId = self->iWindowId;
633 641
634 642 { // assign control id, if id is already in use, try next id
635   - GilSafeSingleLock lock(g_graphicsContext);
  643 + CPyThreadState state;
  644 +
  645 + CSingleLock lock(g_graphicsContext);
636 646 do pControl->iControlId = ++self->iCurrentControlId;
637 647 while (self->pWindow->GetControl(pControl->iControlId));
  648 + lock.Leave();
  649 + state.Restore();
638 650 }
639 651
640 652 // Control Label
@@ -830,15 +842,18 @@ namespace PYXBMC
830 842
831 843 PyObject* Window_GetFocus(Window *self, PyObject *args)
832 844 {
833   - GilSafeSingleLock lock(g_graphicsContext);
834   -
  845 + CPyThreadState state;
  846 +
  847 + CSingleLock lock(g_graphicsContext);
835 848 int iControlId = self->pWindow->GetFocusedControlID();
  849 + lock.Leave();
  850 +
  851 + state.Restore();
836 852 if(iControlId == -1)
837 853 {
838 854 PyErr_SetString(PyExc_RuntimeError, "No control in this window has focus");
839 855 return NULL;
840 856 }
841   - lock.Leave();
842 857
843 858 return (PyObject*)Window_GetControlById(self, iControlId);
844 859 }
@@ -851,8 +866,13 @@ namespace PYXBMC
851 866
852 867 PyObject* Window_GetFocusId(Window *self, PyObject *args)
853 868 {
854   - GilSafeSingleLock lock(g_graphicsContext);
  869 + CPyThreadState state;
  870 +
  871 + CSingleLock lock(g_graphicsContext);
855 872 int iControlId = self->pWindow->GetFocusedControlID();
  873 + lock.Leave();
  874 +
  875 + state.Restore();
856 876 if(iControlId == -1)
857 877 {
858 878 PyErr_SetString(PyExc_RuntimeError, "No control in this window has focus");
@@ -972,8 +992,13 @@ namespace PYXBMC
972 992 return NULL;
973 993 }
974 994
975   - GilSafeSingleLock lock(g_graphicsContext);
  995 + CPyThreadState state;
  996 +
  997 + CSingleLock lock(g_graphicsContext);
976 998 self->pWindow->SetCoordsRes(g_settings.m_ResInfo[res]);
  999 + lock.Leave();
  1000 +
  1001 + state.Restore();
977 1002
978 1003 Py_INCREF(Py_None);
979 1004 return Py_None;
@@ -1056,9 +1081,14 @@ namespace PYXBMC
1056 1081 return NULL; }
1057 1082 if (!key) return NULL;
1058 1083
1059   - GilSafeSingleLock lock(g_graphicsContext);
  1084 + CPyThreadState state;
  1085 +
  1086 + CSingleLock lock(g_graphicsContext);
1060 1087 CStdString lowerKey = key;
1061 1088 string value = self->pWindow->GetProperty(lowerKey.ToLower()).asString();
  1089 + lock.Leave();
  1090 +
  1091 + state.Restore();
1062 1092
1063 1093 return Py_BuildValue((char*)"s", value.c_str());
1064 1094 }
@@ -1091,10 +1121,15 @@ namespace PYXBMC
1091 1121 return NULL;
1092 1122 }
1093 1123 if (!key) return NULL;
1094   - GilSafeSingleLock lock(g_graphicsContext);
1095 1124
  1125 + CPyThreadState state;
  1126 +
  1127 + CSingleLock lock(g_graphicsContext);
1096 1128 CStdString lowerKey = key;
1097 1129 self->pWindow->SetProperty(lowerKey.ToLower(), "");
  1130 + lock.Leave();
  1131 +
  1132 + state.Restore();
1098 1133
1099 1134 Py_INCREF(Py_None);
1100 1135 return Py_None;
@@ -1109,8 +1144,13 @@ namespace PYXBMC
1109 1144
1110 1145 PyObject* Window_ClearProperties(Window *self, PyObject *args)
1111 1146 {
1112   - GilSafeSingleLock lock(g_graphicsContext);
  1147 + CPyThreadState state;
  1148 +
  1149 + CSingleLock lock(g_graphicsContext);
1113 1150 self->pWindow->ClearProperties();
  1151 + lock.Leave();
  1152 +
  1153 + state.Restore();
1114 1154
1115 1155 Py_INCREF(Py_None);
1116 1156 return Py_None;

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.