Bug 44609

Ralf, could you review this please? The patch looks sane to me but I don't speak fluent Windows.

Comment on attachment 55362 [details] [review]
fix logic bug in windows condition variable

Review of attachment 55362 [details] [review]:
-----------------------------------------------------------------

Hmmh, no test case and no receipt to be able to reproduce the problem, which this patch tries to fix. In fact adding this patch enables code, which has been never called before. Because this affects thread logic, which is always complicate, the impact of the code in the if(any) block need to be reviewed too. 


 
-  if (cond->list != NULL)
+  if (any)

Come on, don't be shy.  The condvar_wake_all function isn't even used anywhere in dbus, but only exists to match the POSIX interface for completeness.  Only condvar_wake_one is used.  And that's alright: Broadcasting a condition variable is a rare use case.

So, if you feel more comfortable with that, just rip out the condvar_wake_all implementation entirely.

As for the actual fix and code (just in case there is a lingering interest in having a cond_wake_all implementation in dbus), this is a good resource if you want to learn about the issues involved: http://www.cs.wustl.edu/~schmidt/win32-cv-1.html

Thanks,
Marcus

(In reply to comment #3)
> The condvar_wake_all function isn't even used anywhere in dbus, but
> only exists to match the POSIX interface for completeness.

Well spotted. If this is the case, then its fairness (or lack of) isn't important (downgrading severity accordingly), and I'm inclined to delete the whole thing if we ever break API to drop support for third-party threading implementations (Bug #43744).

Created attachment 57310 [details] [review]
Removed unused condvar_wake_all related code

Comment on attachment 57310 [details] [review]
Removed unused condvar_wake_all related code

Review of attachment 57310 [details] [review]:
-----------------------------------------------------------------

Sorry, we can't do this for API stability reasons.

One thing we *can* do is to remove support for threading implementations
other than those in libdbus (pthreads and Windows), essentially
making dbus_threads_init() equivalent to dbus_threads_init_default();
having done that, we can easily remove support for wake_all (the one
in the struct would be present-but-ignored, but on that branch
so is the rest of the struct). I thought my patches awaiting review on
Bug #43744 already did that, but that must have been in an earlier
version; I'll append a patch 7 that does.

Another possibility would be to remove the functions that *call*
condvar_wake_all, but not the struct member itself, make that
struct member optional (NULL allowed), and remove its Unix and
Windows implementations. I think it's actually better to do
what I proposed on Bug #43744, though.

::: dbus/dbus-threads.h
@@ -172,5 @@
>    DBusCondVarWaitFunction condvar_wait; /**< Function to wait on a condition */
>    DBusCondVarWaitTimeoutFunction condvar_wait_timeout; /**< Function to wait on a condition with a timeout */
>    DBusCondVarWakeOneFunction condvar_wake_one; /**< Function to wake one thread waiting on the condition */
> -  DBusCondVarWakeAllFunction condvar_wake_all; /**< Function to wake all threads waiting on the condition */
> - 

We can't make this change because it breaks ABI (rearranges the struct) - any third-party library supplying its own locking primitives would break.

Fixed by deleting the offending code (but leaving the ABI unchanged), in Bug #43744. 1.5.10