Re: [v3 PATCH 1/2] dwrite: Protect cached fontface list when accessed from multiple threads

[Sebastian Lackner]

On 10.08.2017 08:45, Nikolay Sivov wrote:
> Based on patch by Anton Romanov <theli.ua(a)gmail.com>
>
> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com>
> ---
>  dlls/dwrite/dwrite_private.h |  6 ++++--
>  dlls/dwrite/font.c           | 24 ++++++++++++---------
>  dlls/dwrite/main.c           | 50 +++++++++++++++++++++++++++++++++-----------
>  3 files changed, 56 insertions(+), 24 deletions(-)

Isn't it also necessary to protect factory_cache_fontface()?

By the way: An alternative method to implement proper synchronization (which does
not require locking critical sections before InterlockedDecrement) would be to do
something like:

In the destructor:

--- snip ---
    if (!ref)
    {
        EnterCriticalSection(...);
        list_remove(...);
        LeaveCriticalSection(...);

        ...
    }
--- snip ---

And when querying cached font faces:

--- snip ---
    EnterCriticalSection(...);
    LIST_FOR_EACH_ENTRY(...)
    {
        ...

        /* the following code could also be hidden in QueryInterface */
        if (InterlockedIncrement( &obj->refcount ) == 1)
        {
            InterlockedDecrement( &obj->refcount );
            continue;
        }

        /* return it */
    }
    LeaveCriticalSection(...);
--- snip ---

This is also what is done in Wines threadpool implementation [1], and has the
advantage that the CS is not needed for every Release, only when the object is
really destroyed. But in the end it is a matter of taste, for rpcrt4 Jacek
also picked the "hold CS during InterlockedDecrement" method.

[1] https://source.winehq.org/git/wine.git/blob/HEAD:/dlls/ntdll/threadpool.c#l2...

Best regards,
Sebastian

[v3 PATCH 1/2] dwrite: Protect cached fontface list when accessed from multiple threads

[Nikolay Sivov]

On 10.08.2017 11:14, Sebastian Lackner wrote:
> On 10.08.2017 08:45, Nikolay Sivov wrote:
> > Based on patch by Anton Romanov <theli.ua(a)gmail.com>
> >
> > Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com>
> > ---
> >  dlls/dwrite/dwrite_private.h |  6 ++++--
> >  dlls/dwrite/font.c           | 24 ++++++++++++---------
> >  dlls/dwrite/main.c           | 50 +++++++++++++++++++++++++++++++++-----------
> >  3 files changed, 56 insertions(+), 24 deletions(-)
>
> Isn't it also necessary to protect factory_cache_fontface()?

It is.
> By the way: An alternative method to implement proper synchronization (which does
> not require locking critical sections before InterlockedDecrement) would be to do
> something like:
>
> In the destructor:
>
> --- snip ---
>     if (!ref)
>     {
>         EnterCriticalSection(...);
>         list_remove(...);
>         LeaveCriticalSection(...);
>
> ...
>     }
> --- snip ---

Yes, that's better I guess, thanks.
> And when querying cached font faces:
>
> --- snip ---
>     EnterCriticalSection(...);
>     LIST_FOR_EACH_ENTRY(...)
>     {
>         ...
>
> /* the following code could also be hidden in QueryInterface */
>         if (InterlockedIncrement( &obj->refcount ) == 1)
>         {
>             InterlockedDecrement( &obj->refcount );
>             continue;
>         }
>
> /* return it */
>     }
>     LeaveCriticalSection(...);
> --- snip ---

In my case, to avoid exposing structure, it's enough to AddRef() == 1,
and release otherwise I think. It's not important to decrement when
we're in a process of destroying the object.
> This is also what is done in Wines threadpool implementation [1], and has the
> advantage that the CS is not needed for every Release, only when the object is
> really destroyed. But in the end it is a matter of taste, for rpcrt4 Jacek
> also picked the "hold CS during InterlockedDecrement" method.
>
> [1] https://source.winehq.org/git/wine.git/blob/HEAD:/dlls/ntdll/threadpool.c#l2...
>
> Best regards,
> Sebastian

[v3 PATCH 1/2] dwrite: Protect cached fontface list when accessed from multiple threads

[Anton Romanov]

> --- snip ---
>     if (!ref)
>     {
>         EnterCriticalSection(...);
>         list_remove(...);
>         LeaveCriticalSection(...);
>
> ...
>     }
> --- snip ---
>
> And when querying cached font faces:
>
> --- snip ---
>     EnterCriticalSection(...);
>     LIST_FOR_EACH_ENTRY(...)
>     {
>         ...
>
> /* the following code could also be hidden in QueryInterface */
>         if (InterlockedIncrement( &obj->refcount ) == 1)
>         {
>             InterlockedDecrement( &obj->refcount );
>             continue;
>         }
>
> /* return it */
>     }
>     LeaveCriticalSection(...);
> --- snip ---
> Doesn't the object just leak in this scenario though ? (assuming
> current refcount is 1) :
>  ============= ===========
>    Thread A     Thread B
>  ============= ===========
>                 enter_cs
>                 Increment
>   Decrement
>   ref != Zero
>                 Decrement
>                 leave_cs
>  ============= ===========