mike-jumper commented on a change in pull request #257: GUACAMOLE-958: Avoid
race/deadlock in guacd_timed_client_free()
URL: https://github.com/apache/guacamole-server/pull/257#discussion_r379014428
##########
File path: src/guacd/proc.c
##########
@@ -267,24 +267,24 @@ static int guacd_timed_client_free(guac_client* client,
int timeout) {
.tv_nsec = current_time.tv_usec * 1000
};
- /* Free the client in a separate thread, so we can time the free operation
*/
- if (pthread_create(&client_free_thread, NULL,
- guacd_client_free_thread, &free_operation))
- return 1;
-
/* The mutex associated with the pthread conditional and flag MUST be
* acquired before attempting to wait for the condition */
if (pthread_mutex_lock(&free_operation.completed_mutex))
return 1;
- /* Wait a finite amount of time for the free operation to finish */
- if (pthread_cond_timedwait(&free_operation.completed_cond,
- &free_operation.completed_mutex, &deadline))
- return 1;
+ /* Free the client in a separate thread, so we can time the free operation
*/
+ if (!pthread_create(&client_free_thread, NULL,
+ guacd_client_free_thread, &free_operation)) {
+
+ /* Wait a finite amount of time for the free operation to finish */
+ (void) pthread_cond_timedwait(&free_operation.completed_cond,
Review comment:
I don't think the issue with `pthread_cond_timedwait()` and using a loop
deals specifically with memory barriers, but rather with the potential for the
wait to complete before the expected change is actually made:
>
> ### Condition Wait Semantics
>
> It is important to note that when `pthread_cond_wait() and
`pthread_cond_timedwait()` return without error, the associated predicate may
still be false. Similarly, when `pthread_cond_timedwait()` returns with the
timeout error, the associated predicate may be true due to an unavoidable race
between the expiration of the timeout and the predicate state change.
>
> The application needs to recheck the predicate on any return because it
cannot be sure there is another thread waiting on the thread to handle the
signal, and if there is not then the signal is lost. The burden is on the
application to check the predicate.
>
> Some implementations, particularly on a multi-processor, may sometimes
cause multiple threads to wake up when the condition variable is signaled
simultaneously on different processors.
>
> In general, whenever a condition wait returns, the thread has to
re-evaluate the predicate associated with the condition wait to determine
whether it can safely proceed, should wait again, or should declare a timeout.
A return from the wait does not imply that the associated predicate is either
true or false.
>
> It is thus recommended that a condition wait be enclosed in the equivalent
of a "while loop" that checks the predicate.
>
That said, I think I agree that it's not needed in our case, as the
requirement centers around multiple threads waiting on the condition, whereas
in our case we can be absolutely certain that there is only ever one such
thread.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
