[PATCH v3 6/7] windows.media.speech: Partially implement IAsyncOperation.
Rémi Bernon
rbernon at codeweavers.com
Wed Apr 6 12:26:43 CDT 2022
Hi Bernhard,
On 4/6/22 18:28, Bernhard Kölbl wrote:
> Signed-off-by: Bernhard Kölbl <besentv at gmail.com>
> ---
> v2: Make IAsyncOperation truely asynchronous using a threadpool worker.
> v3: Fix test failures.
> ---
> dlls/windows.media.speech/async.c | 120 ++++++++++++++++++++---
> dlls/windows.media.speech/private.h | 4 +-
> dlls/windows.media.speech/recognizer.c | 10 +-
> dlls/windows.media.speech/tests/speech.c | 70 ++++++-------
> 4 files changed, 158 insertions(+), 46 deletions(-)
>
> diff --git a/dlls/windows.media.speech/async.c b/dlls/windows.media.speech/async.c
> index 94d0b35d9c1..5042a31b503 100644
> --- a/dlls/windows.media.speech/async.c
> +++ b/dlls/windows.media.speech/async.c
> @@ -35,6 +35,18 @@ struct async_operation
> IAsyncInfo IAsyncInfo_iface;
> const GUID *iid;
> LONG ref;
> +
> + IAsyncOperationCompletedHandler_IInspectable *handler;
> + IInspectable *result;
> +
> + async_operation_worker worker;
> + TP_WORK *async_run_work;
> + void *data;
> +
> + CRITICAL_SECTION cs;
> +
> + AsyncStatus status;
> + HRESULT hr;
> };
>
> static inline struct async_operation *impl_from_IAsyncOperation_IInspectable(IAsyncOperation_IInspectable *iface)
> @@ -84,7 +96,11 @@ static ULONG WINAPI async_operation_Release( IAsyncOperation_IInspectable *iface
> TRACE("iface %p, ref %lu.\n", iface, ref);
>
> if (!ref)
> + {
> + if (impl->result) IInspectable_Release(impl->result);
> + DeleteCriticalSection(&impl->cs);
> free(impl);
> + }
>
> return ref;
> }
As described more in detail below, I think you will need to cleanup the
thread pool work item there, calling Cancel and Close, and waiting for
it to complete probably. Or making sure it always completes before the
final async operation Release.
You'll still need to call CloseThreadpoolWork in any case.
> @@ -110,21 +126,60 @@ static HRESULT WINAPI async_operation_GetTrustLevel( IAsyncOperation_IInspectabl
> static HRESULT WINAPI async_operation_put_Completed( IAsyncOperation_IInspectable *iface,
> IAsyncOperationCompletedHandler_IInspectable *handler )
> {
> - FIXME("iface %p, handler %p stub!\n", iface, handler);
> - return E_NOTIMPL;
> + struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(iface);
> + HRESULT hr;
> +
> + TRACE("iface %p, handler %p.\n", iface, handler);
> +
> + EnterCriticalSection(&impl->cs);
> +
> + if (impl->handler)
> + hr = E_ILLEGAL_DELEGATE_ASSIGNMENT;
> + else
> + {
> + impl->handler = handler;
> + if (impl->status != Started && impl->handler)
> + IAsyncOperationCompletedHandler_IInspectable_Invoke(impl->handler, &impl->IAsyncOperation_IInspectable_iface, impl->status);
> + hr = S_OK;
> + }
> +
> + LeaveCriticalSection(&impl->cs);
> +
> + return hr;
> }
I find it surprising, but as far as I can test, a NULL handler is
perfectly valid, but additionally, you can indeed only set the handler
once, even if you've set NULL the first time.
IMHO you don't need to implement that behavior, and instead I think it'd
be better to check that handler parameter is not NULL and return an
error if it is (eventually with a FIXME as it would differ from what
native does).
This would save you the impl->handler check.
>
> static HRESULT WINAPI async_operation_info_Cancel( IAsyncInfo *iface )
> @@ -199,7 +266,28 @@ static const struct IAsyncInfoVtbl async_operation_info_vtbl =
> async_operation_info_Close
> };
>
I think it could be good to implement Cancel and Close too, though as
far as I can see it also means you need to setup a thread pool cleanup
group to able to cancel, close and wait for the work item.
I never used it but there's a sample there:
https://docs.microsoft.com/en-us/windows/win32/procthread/using-the-thread-pool-functions
The idea is that when the async operation is released, you will also
either want it to cancel and wait for the work to complete, so that it
doesn't outlive the async operation and later tries accessing it.
Or, alternatively, you need an additional reference on the async
operation itself while the work is not completed.
As there's a Cancel and Close method I think you'll need the thread pool
cleanup group anyway. Whether native keeps a ref on the async operation
while the work is not complete, or whether it waits for its completion
on async operation final release is a matter of testing but it may be a
bit tricky.
> -HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable **out )
> +static void CALLBACK async_run_cb(TP_CALLBACK_INSTANCE *instance, void *context, TP_WORK *work)
> +{
> + struct async_operation *impl = context;
> + IInspectable *result = NULL;
> + HRESULT hr;
> +
> + hr = impl->worker(impl->data, &result);
> + impl->result = result;
> +
> + EnterCriticalSection(&impl->cs);
> + if (FAILED(hr)) impl->status = Error;
> + else impl->status = Completed;
> +
> + if (impl->handler) IAsyncOperationCompletedHandler_IInspectable_Invoke( impl->handler,
> + &impl->IAsyncOperation_IInspectable_iface,
> + impl->status );
> +
> + impl->hr = hr;
> + LeaveCriticalSection(&impl->cs);
> +}
> +
I find it a bit weird that impl->hr is assigned after the handler has
been invoked. What if the handler calls get_ErrorCode, does it always
get 0 here?
I suspect hr should be set first, so that in the error case the handler
is Invoked with Error status, and can retrieve hr through get_ErrorCode.
> +HRESULT async_operation_create( const GUID *iid, void *data, async_operation_worker worker, IAsyncOperation_IInspectable **out )
> {
> struct async_operation *impl;
>
> @@ -214,6 +302,16 @@ HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable **
> impl->iid = iid;
> impl->ref = 1;
>
> + impl->worker = worker;
> + impl->data = data;
> + impl->async_run_work = CreateThreadpoolWork(async_run_cb, impl, NULL);
> + impl->status = Started;
> +
> + InitializeCriticalSection(&impl->cs);
> + impl->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": async_operation.cs");
> +
> + SubmitThreadpoolWork(impl->async_run_work);
> +
> *out = &impl->IAsyncOperation_IInspectable_iface;
> TRACE("created %p\n", *out);
> return S_OK;
You probably need to handle the potential CreateThreadpoolWork failure.
> diff --git a/dlls/windows.media.speech/recognizer.c b/dlls/windows.media.speech/recognizer.c
> index b16ecb24641..fa36b7937b0 100644
> --- a/dlls/windows.media.speech/recognizer.c
> +++ b/dlls/windows.media.speech/recognizer.c
> @@ -348,12 +348,20 @@ static HRESULT WINAPI recognizer_get_UIOptions( ISpeechRecognizer *iface, ISpeec
> return E_NOTIMPL;
> }
>
> +static HRESULT WINAPI compile_worker( void *data, IInspectable **result )
> +{
> + return S_OK;
> +}
> +
> static HRESULT WINAPI recognizer_CompileConstraintsAsync( ISpeechRecognizer *iface,
> IAsyncOperation_SpeechRecognitionCompilationResult **operation )
> {
> IAsyncOperation_IInspectable **value = (IAsyncOperation_IInspectable **)operation;
> + struct recognizer *impl = impl_from_ISpeechRecognizer(iface);
> +
> FIXME("iface %p, operation %p semi-stub!\n", iface, operation);
> - return async_operation_create(&IID_IAsyncOperation_SpeechRecognitionCompilationResult, value);
> +
> + return async_operation_create(&IID_IAsyncOperation_SpeechRecognitionCompilationResult, impl, compile_worker, value);
> }
>
Although you may want to pass impl there later, as you'll probably need
access to the recognizer in compiler_worker i think you should not pass
it yet, and for instance, pass NULL data, or even better, remove the
data parameter for now.
Later, when you'll need it, you will probably have to increase its
reference count before passing it to async operation, so that the async
operation doesn't outlive it.
Maybe for clarity purposes it'd be even better if data was a reference
counted interface, like an IInspectable, which the async operation can
keep a ref on, itself, in the async_operation_create, and release it on
its destruction.
Cheers,
--
Rémi Bernon <rbernon at codeweavers.com>
More information about the wine-devel
mailing list