[PATCH v4 4/8] server: Defer postprocessing until after setting initial status in send_socket handler.

[Jinoh Kang]

On 3/8/22 03:11, Zebediah Figura wrote:
> 
> 
> On 3/5/22 02:24, Jinoh Kang wrote:
>> On 3/5/22 09:13, Zebediah Figura wrote:
>>> On 3/4/22 13:16, Jinoh Kang wrote:
>>>> On 3/5/22 02:51, Zebediah Figura wrote:
>>>>> On 3/3/22 07:30, Jinoh Kang wrote:
>>>>>> This allows the client to postpone the initial I/O until the server has
>>>>>> queued the I/O request.  The server should perform the postprocessing
>>>>>> only after the initial I/O has been done.
>>>>>>
>>>>>> In the case of send_socket, the manipulation of event flags shall
>>>>>> ideally be done *after* (not *before*) the client has attempted the
>>>>>> initial I/O, since the outbound queue status of the socket may change in
>>>>>> the meanwhile.  Also, the implicitly bound address is available only
>>>>>> after the send* system call has been performed.
>>>>>>
>>>>>> Signed-off-by: Jinoh Kang <jinoh.kang.kr at gmail.com>
>>>>>> ---
>>>>>>
>>>>>> Notes:
>>>>>>        v1 -> v2:
>>>>>>        - pass around total size of data to be transmitted
>>>>>>        - detect short write in send_socket_initial_callback
>>>>>>        v2 -> v3: no changes
>>>>>>        v3 -> v4: no changes
>>>>>>
>>>>>>     dlls/ntdll/unix/socket.c | 15 ++++++++++++++
>>>>>>     server/protocol.def      |  1 +
>>>>>>     server/sock.c            | 42 +++++++++++++++++++++++++++-------------
>>>>>>     3 files changed, 45 insertions(+), 13 deletions(-)
>>>>>>
>>>>>
>>>>> I still am failing to see the need to do this, especially by adding a new callback. What's preventing send_socket from working the same way as recv_socket?
>>>>
>>>> As stated in the commit message, none of the actions in send_socket_initial_callback can be performed *before* the initial I/O.
>>>>
>>>> In send_socket, "send() calls only clear and reselect events if unsuccessful."
>>>> We can tell if the I/O has succeeded only after set_async_direct_result.
>>>> Therefore, we can only clear and reselect events after the client tells us the initial I/O has been unsuccessful.
>>>>
>>> Right, sorry, I'm not thinking this through, or reading apparently...
>>
>> No need to be sorry; I'm sometimes guilty of this too :-/.  I'll try to be more clear the next time.
>>
>>>
>>> I still am not thrilled about adding a new callback, though, at least if it can be avoided.
>>
>> To be fair, extending structs and introducing indirect calls are a pet peeve of mine too.
>>
>>> In this case I wonder if we can make use of sock_reselect_async().
>>>
>>> The exact conditions (and timing) under which we need to clear the AFD_POLL_WRITE bit are not really clear. In particular the correct condition may not be "status != STATUS_SUCCESS" but "status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY". I can't off the top of my head think of any interesting cases where send() returns an error (other than EWOULDBLOCK) but subsequent calls can succeed...
>>
>> Does STATUS_NO_MEMORY count?  ;-)
> 
> Not at all ;-)
> 
>>
>>>
>>> But assuming that we only really need to clear flags when the kernel send buffer is full, I think the right thing to do would be to clear events if "async_waiting( &sock->write_q )".
>>
>> We still need to account for nonblocking I/O (STATUS_DEVICE_NOT_READY), and also is_short_write, which is another indicator for a full send buffer.  Otherwise, it breaks ws2_32:sock:test_write_events.
>> Also, note the implicitly bound address.
>>
>> In any case I think the co-routine pattern is inevitable due to the client-server role split.  In this case, AFD.SYS can do both pre- and post-processing inside a single function, but wineserver can't block for the client to finish the I/O.
>>
>>
> Right. Maybe we could make use of async_set_completion_callback() instead? I think the only reason that can't work is if the sock name needs to be updated and visible *before* the I/O is necessarily complete.

I'll test if Windows does update it before I/O completion.


-- 
Sincerely,
Jinoh Kang