Bug 36204

On Bug #14581, Ralf writes:
> > The default mutex implementations in libdbus are re-entrant with pthreads, and
> > not on win32. :(
> > 
> Can you give some more backgrounds of this assumption ? 
> 
> The CreateMutex doc at
> http://msdn.microsoft.com/en-us/library/ms682411%28v=vs.85%29.aspx says: 
> "... The thread that owns a mutex can specify the same mutex in repeated wait
> function calls without blocking its execution. Typically, you would not wait
> repeatedly for the same mutex, but this mechanism prevents a thread from
> deadlocking itself while waiting for a mutex that it already owns. However, to
> release its ownership, the thread must call ReleaseMutex once for each time
> that the mutex satisfied a wait. ...
> 
> This looks like recursive behavior to me.

It does indeed look like recursive behaviour.

The pthreads implementation and the Windows implementation set different fields in the DBusThreadFunctions struct used by D-Bus: the pthreads implementation supplies recursive mutex functions, and the Windows implementation supplies what claim to be non-recursive mutex functions.

If the Windows implementation is in fact fully recursive, it might be enough to move the position of _dbus_windows_mutex_* in the struct so they fill the "recursive mutex" slots, instead of the deprecated "non-recursive mutex" slots as they do now. I don't develop on Windows, so I can't test this.

Please be careful with this, though, since D-Bus seems to have non-obvious requirements about how mutexes and condition variables interact. dbus-threads.h says:

/** Creates a new recursively-lockable mutex, or returns #NULL if not
 * enough memory.  Can only fail due to lack of memory.  Found in
 * #DBusThreadFunctions. Do not just use PTHREAD_MUTEX_RECURSIVE for
 * this, because it does not save/restore the recursion count when
 * waiting on a condition. libdbus requires the Java-style behavior
 * where the mutex is fully unlocked to wait on a condition.
 */
typedef DBusMutex*  (* DBusRecursiveMutexNewFunction)    (void);

/** Waits on a condition variable.  Found in
 * #DBusThreadFunctions. Must work with either a recursive or
 * nonrecursive mutex, whichever the thread implementation
 * provides. Note that PTHREAD_MUTEX_RECURSIVE does not work with
 * condition variables (does not save/restore the recursion count) so
 * don't try using simply pthread_cond_wait() and a
 * PTHREAD_MUTEX_RECURSIVE to implement this, it won't work right.
 *
 * Has no error conditions. Must succeed if it returns.
 */
typedef void          (* DBusCondVarWaitFunction)        (DBusCondVar *cond,
							  DBusMutex   *mutex);

 * If implementing threads using pthreads, be aware that
 * PTHREAD_MUTEX_RECURSIVE is broken in combination with condition
 * variables. libdbus relies on the Java-style behavior that when
 * waiting on a condition, the recursion count is saved and restored,
 * and the mutex is completely unlocked, not just decremented one
 * level of recursion.

Perhaps those warnings are only applicable to pthreads, and Windows has what Havoc calls the "Java-style" behaviour; I have no idea what semantics Windows gives to mutexes/condvars.

(In reply to comment #1)
> Please be careful with this, though, since D-Bus seems to have non-obvious
> requirements about how mutexes and condition variables interact.
[...]
>  * libdbus requires the Java-style behavior
>  * where the mutex is fully unlocked to wait on a condition.

From a quick look at dbus-sysdeps-thread-win.c, it appears that this is not the case: the Windows implementation only unlocks once to wait for a condition variable, so in the presence of recursive locks, a thread waiting for a condvar can deadlock itself, by not letting the thread that would signal the condvar run. (Unless I'm reading it wrong, which is entirely possible.)

You'd probably have to turn DBusMutex into a struct containing the underlying HANDLE, the thread ID of the holder, and the recursion count, like dbus-sysdeps-pthread does... I think.

I wonder whether libdbus should just assume that the platform only provides non-recursive locks, implement the recursion itself (like we already do for pthreads!), and thus get recursive behaviour even if we're only given a non-recursive mutex implementation? But I'm put off by the fact that there's a comment that says "Not 100% sure this is safe". That's really not what you want to see when dealing with threads!

Does anyone know which parts of D-Bus rely on those "Java-style" semantics, and why? If none of our target platforms provide them, then they seem pretty useless semantics to expect.

I hate threads. :-(

Fixed in 1.5.10, I believe. If our Windows mutexes are secretly broken, hopefully it won't take as long to find out as it did for pthreads (which were broken from 2006 to 2012).