Hi Bernhard,
On 4/6/22 18:28, Bernhard Kölbl wrote:
Signed-off-by: Bernhard Kölbl besentv@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-p...
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,