[Development] QBasicMutex::lockInternal() race condition?

[Tony Van Eerd]

> -----Original Message-----
> > I have a change to document more the internal, but it is not merged
> yet.
> > https://codereview.qt-project.org/33739
> 
> Staged now, thanks :-)

It would be nice if the new comments on the if (d->ref()) line pointed out that 'd' is never invalid because we recycle, but never free, the pointers.

> 
> Tony: I'd really appreciate giving the implementation a thorough once-
> over.

I will try to find the time, although I'm already further down the rabbit hole than I should be.  I was really investigating what can be used in a static context, and what can't.  Would you recommend that client code use QBasicMutex when static mutexes are needed (which is a common use for mutexes).  Like the recent changes done for Qt internals?  (P.S. I noticed one of those had a static QBasicMutex inside a function, which gcc will then put a 'mutex' around...)

The 'possiblyUnlocked' flag is worrisome - whenever you add another state/flag, you exponentially increase the number of possible states that need checking (in general).  I'm not saying I see an actual problem with the flag in the current implementation, just that it would be a place to look :-)

P.S. although I agree that recursive mutex is often wrong, it is commonly used.  Possibly the most common, particularly for OOP and ex-java developers.  (And to me, Qt is very java-esque, no offense.)


> I've done it once and, after rewriting it, I came up with almost the
> same
> solutions. My implementation only differed that I added a spinlock, by
> using
> the lowest bit on the QMutexPrivate pointer.
> 

did you use "testAndtestAndSet" ?

while (locked || !testAndSet(&locked, 0, 1))
   yield-or-whatever;

ie mostly spin on a relaxed read.  Only do the memory-barrier CAS when it "looks" unlocked, and thus has a chance of succeeding.



> I don't think my solution is any better than Olivier's. Both need to
> spin and
> retry: mine with a spinlock, his with that reference-and-recheck you
> found and
> the increment-from-zero in ref() one. I've given some thought on how to
> improve on this, but I haven't come up with any better solutions.
> 
> If you do have any idea, let us know. And please do it quickly :-)
> 
> It is also acceptable to increase the size of QMutex by another
> pointer, if
> that improves the solution considerably.

I'd agree with that.  If only I had a solution that fit that description.

In general, this problem seems the same as the pthread_once(), so various solutions are out there.  Lawrence Crowl had a good solution using thread-local-storage.  It is used in boost::call_once() for the non-Windows implementation.  The Windows impl is similar to QMutex in that it tries to avoid CreateEvent (like trying to avoid allocating the d_ptr) unless there is contention.  But the boost version has a small case where it never cleans up the event (which is OK for once(), since it tends to be a static that is never cleaned up anyhow).  I had a solution to that small case, but it took about 5 pages of code (lots of comments of course).  And used a list built across objects on the stacks of the waiting threads.  Which is scary if someone decides to terminate a thread.  But it showed that you could do everything with a single int, if you really wanted to.  I think I had a solution with 2 ints without the list...


---------------------------------------------------------------------
This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful.