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 July 22, 2012

... 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 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 July 23, 2012

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 July 23, 2012

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 October 11, 2012
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 23, 2012
Chris Browet 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.
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;
Commit_comment_tip

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.