Re: [AD] timer_mutex again

[Peter Wang <tjaden@xxxxxxxxxx>]

Eric Botcazou wrote:

> > So thread 4 is holding the timer_mutex in _handle_timer_tick() and
> > wanting to get the _xwin.mutex.  Meanwhile thread 1 has the _xwin.mutex
> > (due to an acquire_screen in dialog_message) and wants the timer_mutex.
> >    
>
> Thanks.  I think dialog_message is wrong, it should not acquire_screen.  
> AFAICS object_message already has the necessary bits.
>
> scare_mouse clearly cannot be invoked when the screen has been acquired.
>  

Ah, cheers!  Attached is a patch for dialog_message() instead, with a 
verbose explanation.

> > > The idea was to make sure that all callbacks have completed before
> > > remove_int returns.  Does your proposed change still guarantee that?
> > >      
> > No :-(  Actually, the same problem is marked FIXME in the new_api_branch
> > so I should have remembered it.
> >    
>
> I should have documented it in timer.c.
>  
Patch attached; what do you think?

Peter

Index: src/gui.c
===================================================================
RCS file: /cvsroot/alleg/allegro/src/gui.c,v
retrieving revision 1.74
diff -u -r1.74 gui.c
--- src/gui.c	7 Apr 2005 21:59:36 -0000	1.74
+++ src/gui.c	18 Apr 2005 12:13:23 -0000
@@ -364,8 +364,21 @@
    DIALOG *menu_dialog = NULL;
    ASSERT(dialog);
 
-   if (msg == MSG_DRAW)
-      acquire_screen();
+   /* Note: don't acquire the screen in here.  A deadlock develops in a
+    * situation like this:
+    *
+    * 1. this thread: acquires the screen;
+    * 2. timer thread: wakes up, locks the timer_mutex, and calls a
+    *    callback to redraw the software mouse cursor, which tries to
+    *    acquire the screen;
+    * 3. this thread: object_message(MSG_DRAW) calls scare_mouse()
+    *    which calls remove_int().
+    *
+    * So this thread has the screen acquired and wants the timer_mutex,
+    * whereas the timer thread holds the timer_mutex, but wants to acquire
+    * the screen.  The problem is calling scare_mouse() with the screen
+    * acquired.
+    */
 
    force = ((msg == MSG_START) || (msg == MSG_END) || (msg >= MSG_USER));
 
@@ -409,9 +422,6 @@
 	 break;
    }
 
-   if (msg == MSG_DRAW)
-      release_screen();
-
    return res;
 }
 

Index: src/timer.c
===================================================================
RCS file: /cvsroot/alleg/allegro/src/timer.c,v
retrieving revision 1.20
diff -u -r1.20 timer.c
--- src/timer.c	14 Mar 2005 11:41:59 -0000	1.20
+++ src/timer.c	18 Apr 2005 12:27:01 -0000
@@ -50,6 +50,16 @@
 
 #ifdef ALLEGRO_MULTITHREADED
 static void *timer_mutex = NULL;          /* global timer mutex */
+   /* Why we need timer_mutex:
+    *
+    * 1. So _timer_queue[] is not changed in the middle of
+    * _handle_timer_tick's loop, possibly causing a null pointer
+    * dereference or corruption;
+    *
+    * 2. So that after "remove_int(my_callback);" returns, my_callback
+    * is guaranteed not to be in the _timer_queue and will not be
+    * called from the timer thread.
+    */
 #else
 static int timer_semaphore = FALSE;       /* reentrant interrupt? */
 #endif