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

[Rémi Bernon]

On 4/20/22 18:29, Bernhard Kölbl wrote:
> 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?
> 

I think the initial status, should be changed, in this patch, to 
Started, and it then doesn't need to be set here (only check if the 
operation has already been cancelled).


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


As stated below I think you still need to invoke the handler, even in 
the Canceled state, you don't need to care about whether the handler 
will take long or not.

In any case, this specific condition here will never be true: you just 
wrote either Error or Completed to impl->status, inside the critical 
section, so it cannot be Canceled there.

If you want to keep the Canceled state here, after the callback has 
executed, you need to check it and keep it before setting Error / Completed.


>>
>>> +
>>> +    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-- 
Rémi Bernon <rbernon at codeweavers.com>