[Development] Reviews needed before android integration in two weeks

[Olivier Goffart]

On Wednesday 06 February 2013 12:38:12 Thiago Macieira wrote:
> On quarta-feira, 6 de fevereiro de 2013 21.13.36, Olivier Goffart wrote:
> > That said, it is probably a good idea not to call select at all when it is
> > not  needed.  But with that patch, you also make calls to write even when
> > it is not needed, while before there was only call to write.
> 
> The atomic is there to avoid a write to the pipe/eventfd when it has already
> been written to. That is, in QEventDispatcherUNIX::wakeUp, it saves a
> syscall if the target thread has already been told to wake up.
> 
> In QEventDispatcherUNIXPrivate::processThreadWakeUp, it will read from the
> pipe/eventfd and then reset the atomic back to zero, indicating that a write
> is necessary again.
> 
> Here's how an event is posted (thread A is the thread that does dispatching,
> thread B is the one posting the event):
> 
>  - thread A is awake in QEventDispatcherUNIXPrivate::processThreadWakeUp
>  - thread B is in QEventDispatcherUNIX::wakeUp
>  - thread A has just returned from read(2) but hasn't reset the atomic yet
>  - (1) thread B is checking the atomic and finds it at 1
>  - (2) thread A resets the atomic
>  - thread B will then proceed without writing to the eventfd/pipe
> 
> All events in a given thread happen in the order that they are listed.
> Events in separate threads can happen in any order relative to each other,
> *except* for (1) and (2): must happen in that order because the reverse is
> not possible.
> 
> The above could be a problem, but isn't because it depends on external
> factors. Namely:
> 
>  - wakeUp() is only called from qcoreapplication.cpp with the event queue
>    mutex locked

wakeUp is called from many places, and without the lock.

It seems to be called from QCoreApplicationPrivate::sendPostedEvents with the 
mutex locked, but that is a bug. We should unlock that mutex before calling 
wakeUp,  as wakeUp is user code and should not be called with the mutex hold.

>  - it's only called *after* the event queue has been modified in postEvent
>  - processThreadWakeUp is only called *before* the event queue is inspected

Yes, and that should garentee that QEvent get properly processed and that 
there is no race condition.


> That means these two events are necessarily ordered:
> 
>  - thread B adds an event to thread A's event queue
>    [happens before "thread B is in QEventDispatcherUnix::wakeUp",
>     which happens before "thread B is checking the atomic and finds it at
> 1"] - thread A processes its event queue
>    [happens after "thread A resets the atomic"]
> 
> This proves that the event posting is correct. Empirical evidence (namely,
> the lack of bug reports concerning this problem) also supports that
> conclusion.
> 
> If you're finding a problem, its root cause must be elsewhere and must be
> dealt there.

Maybe there are other events?  integration with a non-Qt event loop?

-- 
Olivier

Woboq - Qt services and support - http://woboq.com