[PATCH v3 3/8] server: Defer postprocessing until after setting initial status in recv_socket handler.

[Zebediah Figura (she/her)]

On 3/1/22 06:42, Jinoh Kang wrote:
> On 2/26/22 03:42, Zebediah Figura (she/her) wrote:
>> On 2/21/22 11:23, Jinoh Kang wrote:
>>> This allows the client to postpone the initial I/O until after the
>>> server has actually queued the I/O request.  The server should perform
>>> the postprocessing only after the initial I/O has been done.
>>>
>>> In the case of recv_socket, the manipulation of event flags shall
>>> ideally be done *after* (not *before*) the client has attempted the
>>> initial I/O, since the readiness of the socket may change in the
>>> meanwhile.
>>>
>>> Signed-off-by: Jinoh Kang <jinoh.kang.kr at gmail.com>
>>
>> What's the point of doing this?
> 
> Clarity, mostly.
> 
> Clearing pending_events indicates that we're interested in polling I/O again.
> Since the recv() call has not yet happened, it means the incoming queue may still have pending data.
> One might reason that this will erroneously trigger AFD_POLL_READ event, even though the recv() call will consume all the remaining data.
> 
> Now, the above scenario won't really happen since we never poll for POLLIN if async_waiting( &sock->read_q ) is false.
> I nonetheless find it much more intuitive to have the event flag cleared *after* performing the initial I/O.
> This also makes it consistent with the send_socket case.

Well, we can't clear event flags after performing the initial I/O per 
se. At best we can do so after *queuing* it, which isn't particularly 
meaningful. Perhaps there's an argument for moving the flag manipulation 
after queue_async(), but I'll admit it doesn't really seem worthwhile. I 
don't see the point of moving it after async_handoff(), though.

Note also that we have to clear event flags even in (some?) error cases, 
where no I/O is really queued. So while I can only assume this passes 
the tests, it makes it much less obvious that it's the right thing to 
do. It also introduces a mental burden on the reader to understand the 
semantics of the "initial status callback" and when (and under what 
circumstances) it's called.