[RFC PATCH v2 2/5] server: Add a new server request "notify_async_direct_result."

[Zebediah Figura (she/her)]

On 1/27/22 13:22, Jinoh Kang wrote:
> On 1/28/22 04:13, Jinoh Kang wrote:
>> On 1/28/22 04:05, Jinoh Kang wrote:
>>> +
>>> +/* Notify direct completion of async and close the wait handle */
>>> +DECL_HANDLER(notify_async_direct_result)
>>> +{
>>> +    struct async *async = (struct async *)get_handle_obj( current->process, req->handle, 0, &async_ops );
>>> +
>>> +    if (!async) return;
>>> +
>>> +    if (async->iosb && async->unknown_status && !async->pending && async->terminated)
>>> +    {
>>> +        /* Reactivate async.  We call async_reselect() later. */
>>> +        async->terminated = 0;
>>> +
>>> +        /* Set result for async_handoff(). */
>>> +        set_error( req->status );
>>> +        async->iosb->result = req->information;
>>> +
>>> +        /* The async_handoff() call prior to the current server request was
>>> +         * effectively a no-op since async->unknown_status is 1.  Calling it
>>> +         * again with async->unknown_status = 0 will do the remaining steps.
>>> +         */
>>> +        async->unknown_status = 0;
>>> +        async_handoff( async, NULL, 0 );
>>> +
>>> +        if (get_error() == STATUS_PENDING)
>>> +        {
>>> +            async_reselect( async );
>>> +        }
>>> +        else if (NT_ERROR( get_error() ))
>>> +        {
>>> +            /* synchronous I/O failure: don't invoke callbacks, only dequeue it. */
>>> +            async_dequeue( async );
>>
>> Note: we can't use async_set_result() here.  async_handoff() leaves async->terminated as 0, and async_set_result() has "assert( async->terminated )".
>>
>> Thus, reusing async_set_result() here requires either:
>>
>> 1. Setting async->pending to 0 so that async_handoff() will call async_terminate().
>>     This is obviously incorrect since unwanted IOCP packets and APCs will fire on synchronous failure.
>>
>> 2. Not using async_handoff().  We have to copy a lot of code (particulary setting pending and direct_result flags) out of async_handoff() to do this.
> 
> Maybe just merge both branches (error/success) and force async_terminate() before calling async_set_result()?

Actually, I believe the right thing to do is just to set 
async->terminated = 1. In fact, I believe you should set 
async->terminated = 1 in the recv_socket request, otherwise 
async_waiting() will return 1 and the server will end up spinning and/or 
trying to wake the async. (Granted, that could be protected against by 
checking unknown_status, but...)

There are a few different ways to look at this, but one way (and what I 
think makes the most sense to me) is that we're completing the async as 
if it had been alerted, much as we would from a kernel APC, except that 
we're also setting the initial status, which wasn't known yet. Hence we 
want to go through async_set_result().

Along these lines, if you also set async->alerted = 1, you get the 
STATUS_PENDING "restart the wait" behaviour for free. Which makes 
conceptual sense as well.

I suppose this is why you mentioned direct_result in [1]—i.e. we could 
just async_terminate(STATUS_ALERTED) in recv_socket. I can see both 
approaches working, but I think that open-coding the relevant parts of 
async_terminate would be better than trying to get direct_result 
manipulation exactly right. Of course, I also haven't tried...

[1] https://www.winehq.org/pipermail/wine-devel/2022-January/205774.html