[PATCH v4 3/4] windows.media.speech: Implement concurrency in IAsyncOperation.
Bernhard Kölbl
besentv at gmail.com
Wed Apr 20 11:56:16 CDT 2022
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>
More information about the wine-devel
mailing list