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

[Rémi Bernon]

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".


> +    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?


> +
> +    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.


> +
> +    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.


> +    }
> +    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.


-- 
Rémi Bernon <rbernon at codeweavers.com>