[PATCH v4 3/4] windows.media.speech: Implement concurrency in IAsyncOperation.

[Bernhard Kölbl]

Am Di., 19. Apr. 2022 um 15:08 Uhr schrieb Rémi Bernon
<rbernon at codeweavers.com>:
>
> On 4/18/22 23:42, Bernhard Kölbl wrote:
> > Signed-off-by: Bernhard Kölbl <besentv at gmail.com>
> > ---
> >   dlls/windows.media.speech/async.c        | 123 ++++++++++++++++++++++-
> >   dlls/windows.media.speech/private.h      |   4 +-
> >   dlls/windows.media.speech/recognizer.c   |   7 +-
> >   dlls/windows.media.speech/tests/speech.c |  22 ++--
> >   4 files changed, 141 insertions(+), 15 deletions(-)
> >
> > diff --git a/dlls/windows.media.speech/async.c b/dlls/windows.media.speech/async.c
> > index cf0da23e493..66cba46d85b 100644
> > --- a/dlls/windows.media.speech/async.c
> > +++ b/dlls/windows.media.speech/async.c
> > @@ -40,7 +40,13 @@ struct async_operation
> >       LONG ref;
> >
> >       IAsyncOperationCompletedHandler_IInspectable *handler;
> > +    IInspectable *result;
> >
> > +    async_operation_worker worker;
>
>
> IMHO "callback" is more commonly used and more appropriate name than
> "worker".
>

Sure. I used "worker" because the threadpool callback is already named
like that, so a possbile confusion would be eliminated.

>
> > +    TP_WORK *async_run_work;
> > +    IInspectable *invoker;
> > +
> > +    CRITICAL_SECTION cs;
> >       AsyncStatus status;
> >       HRESULT hr;
> >   };
>
>
>
> > @@ -299,7 +345,65 @@ static const struct IAsyncInfoVtbl async_operation_info_vtbl =
> >       async_operation_info_Close
> >   };
> >
> > -HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable **out )
> > +static void CALLBACK async_run_cb(TP_CALLBACK_INSTANCE *instance, void *data, TP_WORK *work)
> > +{
> > +    IAsyncOperation_IInspectable *operation = data;
> > +    struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(operation);
> > +    IInspectable *result = NULL, *invoker = NULL;
> > +    async_operation_worker worker;
> > +    HRESULT hr;
> > +
> > +    EnterCriticalSection(&impl->cs);
> > +    if (impl->status == Canceled)
> > +        goto abort;
> > +
> > +    impl->status = Started;
>
>
> Should it really ignore and overwrite the Closed status here?
>
> Or perhaps instead treat it as cancellation, as it means the client
> probably doesn't care about the result anymore?

Right, I use Closed as the default/init state, that's why it is
overwritten here.
So not sure if it needs a change?

>
> > +
> > +    invoker = impl->invoker;
> > +    worker = impl->worker;
> > +    LeaveCriticalSection(&impl->cs);
> > +
> > +    hr = worker(invoker, &result);
> > +
> > +    EnterCriticalSection(&impl->cs);
> > +    if (FAILED(hr))
> > +        impl->status = Error;
> > +    else
> > +        impl->status = Completed;
> > +
> > +    impl->result = result;
> > +    impl->hr = hr;
> > +
> > +    if (impl->status == Canceled)
> > +        goto abort;
>
>
> This condition will never be true as you just wrote the status above. I
> think you can consider that if the callback had been called it's too
> late for it to be cancelled.
>

The idea is to cancel the invoication of the handler, i.e. leaving the
thread asap.
The condition can be true if you call cancel during the call to worker/callback.

>
> > +
> > +    if (impl->handler != NULL && impl->handler != HANDLER_NOT_SET)
> > +    {
> > +        IAsyncOperationCompletedHandler_IInspectable *handler = impl->handler;
> > +        AsyncStatus status = impl->status;
> > +
> > +        IAsyncOperation_IInspectable_AddRef(operation);
> > +        IAsyncOperationCompletedHandler_IInspectable_AddRef(handler);
> > +        LeaveCriticalSection(&impl->cs);
> > +
> > +        IAsyncOperationCompletedHandler_IInspectable_Invoke(handler, operation, status);
> > +
> > +        IAsyncOperationCompletedHandler_IInspectable_Release(handler);
> > +        IAsyncOperation_IInspectable_Release(operation);
>
>
> I don't think you need to add all these references here, you already
> hold one on the operation and on the handler.

Yeah.

>
>
> > +    }
> > +    else
> > +        LeaveCriticalSection(&impl->cs);
> > +
> > +    IAsyncOperation_IInspectable_Release(operation);
> > +    return;
> > +
> > +abort:
> > +    impl->hr = E_FAIL;
> > +    LeaveCriticalSection(&impl->cs);
> > +    IAsyncOperation_IInspectable_Release(operation);
> > +}
> > +
>
>
> It will be hard to check, but I think you need to invoke the handler
> even in the Cancel case, as MSDN suggests:
>
> > If you're implementing IAsyncOperation<TResult>, then the set
> > implementation of Completed should store the handler, and the
> > surrounding logic should invoke it when Close is called. The
> > implementation should set the asyncStatus parameter of invoked
> > callbacks appropriately if there is a Cancel call, Status is not
> > Completed, errors occurred, and so on.
>

Yeah, seems odd but I can change it.

Bernhard