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

[Bernhard Kölbl]

Am Mi., 20. Apr. 2022 um 18:42 Uhr schrieb Rémi Bernon
<rbernon at codeweavers.com>:
>
> 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.

My bad, I mistook the assignments for compares.
I'll make it so Canceled will be kept and the handler is always called.

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