[PATCH 4/4] dlls/ntdll: Call try_recv inside sock_recv only if it is safe to do so.
Zebediah Figura (she/her)
zfigura at codeweavers.com
Tue Jan 18 22:52:58 CST 2022
On 1/18/22 20:30, Jinoh Kang wrote:
> Actually I have observed that simply removing try_recv() altogether from
> sock_recv() suffers from another set of problems (that make the test fail):
>
> 1. It makes recv() on nonblocking socket always fail with WSAEWOULDBLOCK.
> 2. It turns all synchronous failures into asynchronous failures. Normally,
> a synchronous failure shall not fire up the APC or post to the completion
> port at all.
>
> Thus, a call to try_recv() still has to be made before the async request enters
> pending mode, regardless of whether this call is done before receive_sock or
> not.
>
> My previous approach was to do this "initial" try_recv() call via APC_ASYNC_IO
> (i.e. inside async_recv_proc), thereby making the call just like any other
> asynchronous try_recv() call. Eventually I could get to the point where all the
> unit tests pass, but the end result looked a bit messy (especially with the
> special case handling inside invoke_system_apc, and extra arrangements that are
> needed inside receive_sock).
Okay, that's fair enough.
>> That aside, though, I'm not sure that adding a new select type is really the right way to go about this. One approach that occurs to me, which might end up being simpler, would be to return an apc_call_t, essentially as if the select request had been called immediately afterward. (In that case perhaps we should return STATUS_KERNEL_APC rather than STATUS_ALERTED).
>
> This is exactly the approach I have initially had in mind, and is described in
> https://bugs.winehq.org/show_bug.cgi?id=52401#c4. Pasting it here:
>
>> 1. The client issues a 'recv_socket' call.
>> 2. The server determines if read_q is empty *and* the request can be served
>> immediately.
>> 3. If this is the case, the server does the following:
>> 1. The server marks the current thread as being in system APC wait.
>> 2. The server bypasses sock->read_q and calls async_terminate() directly.
>> (Alternatively, the server first puts the async in read_q and later
>> calls async_wake_up( &sock->read_q ).)
>> 3. The server dequeues the next system APC (likely APC_ASYNC_IO from 3.2)
>> from the queue.
>> 4. The server inserts the APC into the 'recv_socket' reply.
>>
>> Ideally we could make it so that every Wineserver call has a flag that indicates
>> whether it could process the original request *and* retrieve the next system APC
>> from the queue in one go. This will eliminate the extra SIGUSR1 round trip in
>> other cases as well.
>
> (Note: 3.2 is slightly wrong, we should always go through sock->read_q no matter
> what.)
>
> As you can see this inevitably winds up tweaking the select mechanism
> anyway. This approach has some open questions:
>
> 1. How shell we return the results for our new pseudo-APC call?
> Using prev_apc in the select request, or a separate server call?
Using select seems reasonable to me. I don't think it would require much
change in ntdll [an extra argument to server_select(), probably], and
none at all in the server.
>
> 2. Shall our pseudo-APC go through the system_apc queue?
> If so, what if there is another kernel APC that is already queued?
Yes, the point is to use APC_ASYNC_IO itself, so we just reuse that
entire code.
I would imagine we should return the first APC in the queue. In the
normal case that should be the APC we just queued; if not we were going
to receive SIGUSR1 anyway.
> 3. Shall we reuse invoke_system_apc() for this apc_call_t response?
Yes, of course.
> 4. Shall we reuse queue_apc() to generate the apc_call_t?
> If so, what shall be done with the gratuitous SIGUSR1 delivery?
> Enter uninterrupted region or explicitly block SIGUSR1 while
> calling receive_sock?
I think we should avoid sending SIGUSR1 in the first place. We'd need
some way of communicating to queue_apc() or even is_in_apc_wait() that
we are going to try to return the APC synchronously.
>
> Also, as I've discussed earlier, the initial synchronous try_recv() call has
> subtly different semantics with respect to error handling compared to
> subsequent asynchronous counterparts, which does make existing code more
> complicated.
Sure. I don't think using APC_ASYNC_IO should complicate any of that,
although to be sure I haven't tried it.
More information about the wine-devel
mailing list