Code review comment for lp:~albaguirre/mir/fix-tsan-findings

> >As display_listener is const, isn't this simpler as:
>
> > auto const disp_listener = display_listener;
>
> The MP code I propose avoids an unnecessary ref count increase, but sure I can
> make that change.

Don't we get a single ref count increment in both cases? The proposed version can't move from a const display_listener so an unnamed temporary is created for the move.

> >Or even better, just make it a value capture in the lines below? Vis:
>
> You cannot capture a member variable directly; you can only through access
> through "this" (or by creating a local reference that the lambda can then
> capture).

I gave the syntax I was thinking of. Admittedly, that does create two copies of display_listener.

~~~~

> > So we no longer use "on_unwind()" because the destructor of the moved
> promise
> > will throw after we set_exception()? And then we eat the exception anyway?
> > That seems an overly complex mechanism to destroy
> CompositingFunctor::promise.
>
> No. I didn't see a need for unwind helper since we already have a catch block.
>
> The main motivation is not removing the unwind helper, it's removing the query
> of std::future since it's not threadsafe (std::future sate would be accessed
> by two threads). Rather than synchronizing that access, I avoid querying the
> std::future at all. Since I won't check its status, then set_exception may
> throw if it already has a value and so we eat the exception.
>
> The move of the std::promise is also to avoid two threads access std::promise
> state, the thread calling the destructor and the thread accessing the promise
> (the compositing thread).

I need to think about this - it sounds like the wrong synchronization objects are being used.