Re: svn commit: r1615349 - in /subversion/trunk/subversion: include/svn_error_codes.h libsvn_subr/mutex.c

On 04.08.2014 15:22, Ivan Zhakov wrote:
> On 2 August 2014 23:13, <stefan2_at_apache.org> wrote:
> @@ -195,7 +206,9 @@ svn_mutex__unlock(svn_mutex__t *mutex,
> #else
> /* Update lock counter. */
> if (mutex->checked)
> - --mutex->count;
> + if (--mutex->count)
> + return svn_error_create(SVN_ERR_INVALID_UNLOCK, NULL,
> + _("Tried to release a non-locked mutex"));
> 1. Why you are using non-atomic decrement here while we're using
> casptr() for mutex->owner?

This bit of code is only used when APR does *not* support threads, IIRC.
So atomic insns are overkill.

> 2. You're leaving mutex object in invalid state on attempt to release
> non-locked mutex.

That's been fixed.

> I also find checked mutexes a 'false sense of security' thing: we
> should be more careful when writing multi-threaded code instead of
> relying on unreliable protection mechanism. That is because many
> multi-threading problems do not have stable reproduction scripts

I feel the same about that ... I'd have recommended to just use the
APR_THREAD_MUTEX_UNNESTED flag, but then on Windows, this uses an event
instead of a critical section.

-- Brane

-- 
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane_at_wandisco.com