[vlc-devel] [PATCH] darwin/thread: Ignore vlc_mutex_unlock failures

[David Fuhrmann]

> Am 28.10.2019 um 18:24 schrieb Marvin Scholz <epirat07 at gmail.com>:
> 
> macOS pthread implementation for pthread_cond_wait is buggy, causing
> sometimes the mutex to not be locked when running thread cancellation
> cleanup handlers.
> This causes random failures of vlc_mutex_unlock, as the cancellation
> cleanup handler does not hold a lock on the mutex in rare cases.
> ---
> src/darwin/thread.c | 27 ++++++++++++++++++++++++++-
> 1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/src/darwin/thread.c b/src/darwin/thread.c
> index 9dcffad28d..10bcae9e28 100644
> --- a/src/darwin/thread.c
> +++ b/src/darwin/thread.c
> @@ -172,7 +172,32 @@ int vlc_mutex_trylock (vlc_mutex_t *p_mutex)
> void vlc_mutex_unlock (vlc_mutex_t *p_mutex)
> {
>     int val = pthread_mutex_unlock( p_mutex );
> -    VLC_THREAD_ASSERT ("unlocking mutex");
> +    /* FIXME: We can't check for the success of the unlock
> +     * here as due to a bug in Apples pthread implementation.
> +     * The `pthread_cond_wait` function does not behave like
> +     * it should According to POSIX, pthread_cond_wait is a
> +     * cancellation point and when a thread is cancelled while
> +     * in a condition wait, the mutex is re-acquired before
> +     * calling the first cancellation cleanup handler:
> +     *
> +     * > The effect is as if the thread were unblocked, allowed
> +     * > to execute up to the point of returning from the call to
> +     * > pthread_cond_timedwait() or pthread_cond_wait(), but at
> +     * > that point notices the cancellation request and instead
> +     * > of returning to the caller of pthread_cond_timedwait()
> +     * > or pthread_cond_wait(), starts the thread cancellation
> +     * > activities, which includes calling cancellation cleanup
> +     * > handlers.
> +     *
> +     * Unfortunately the mutex is not locked sometimes, causing
> +     * the call to `pthread_mutex_unlock` to fail.
> +     * Until this is fixed, enabling this assertion would lead to
> +     * spurious test failures and VLC crashes when compiling with
> +     * debug enabled, which would make it nearly impossible to
> +     * proeprly test with debug builds on macOS.
> +     * This was reported to Apple as FB6152751.
> +     */
> +    // VLC_THREAD_ASSERT ("unlocking mutex");
>     vlc_mutex_unmark(p_mutex);
> }

Hi,

Change is ok for me, for a simple reason: This issue is bugging us since years now, and apparently is due to changes in the macOS internals we cannot influence or fix.

As an end result, at least I as a mac developer am blocked in ever enabling debug builds locally, because it would crash sooner or later. As a consequence, we might miss lots and lots of other valid assertions I would actually like to see.

BR. David