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

[Zebediah Figura]

Sorry for being slow to respond...

On 1/27/22 22:17, Jinoh Kang wrote:
>>>> 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,
> 
> Yes, it's already done in async_handoff().  Note the precondition above:
> 
>> +    if (async->iosb && async->unknown_status && !async->pending && async->terminated)
>> +    {
>> +        /* Reactivate async.  We call async_reselect() later. */
>> +        async->terminated = 0;
> 
>> 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'm honestly split off as to which side should take the responsibility of setting "async->alerted = 1", async_handoff() or notify_async_direct_result.
> I suppose I'm going with the former, since that's where STATUS_ALERTED is received. 

I think that inasmuch as we're mimicking the usual "alerted" code path, 
it makes conceptual sense to set async->alerted = 1 in async_handoff().

> But then, shall the latter have "assert( async->alerted );"...?

Well, you can't do that, exactly, because we can't crash no matter what 
input we get from the client. But you could check for async->alerted and 
return STATUS_INVALID_PARAMETER or something if it's not set.

>> 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.
> 
> I agree.  I hope we don't also have to also open code "async_handoff()", though.
> 
>> Of course, I also haven't tried...
>>
>> [1] https://www.winehq.org/pipermail/wine-devel/2022-January/205774.html
> 
> That said, the more I work with the async code, the more I learn to hate all those status bitfields...
> I think it's much better off merging as many states as possible into a single enum, so that the lifecycle is explicit (e.g. ASYNC_INITIAL -> ASYNC_QUEUED -> ASYNC_ALERTED -> ...)
> (I'm aware it was much worse before that, making the status field serve the dual role of async state and I/O status.)
> 

Sure, I think that's something worth looking into.