[RFC PATCH v4 0/5] Fix sock_recv reordering issue (#52401): unknown_status + wineserver call approach

[Jinoh Kang]

<preamble>

This is my forth attempt to tackle the issue #52401 [1]: "Improper
synchronization in sock_recv/sock_send leads to arbitrary reordering of
completion of I/O requests", which was initially reported (outside Bugzilla)
by Dongwan Kim [2].

Basically, the old process of sock_recv is:

1. Perform the I/O synchronously.
2. Report the result back to the server,
   or queue up the request in the server.
3. If blocking, wait for I/O to complete.

The new process of sock_recv would be:

1. Queue up the request in the server.
2. Perform the I/O synchronously.
3. Report the result back to the server,
4. If blocking, wait for I/O to complete.

Everything except the actual I/O requires communicating with wineserver.
My goal here is to fix the issue without introducing extra calls to wineserver.
(However, if it turns out that this goal is not of utmost significance, then
 this patch serie can be easily modified so that it issues separate server
 calls.)

The previous approaches are listed here:

- Add a new select type called SELECT_SIGNAL_WAIT_ASYNC [3].

  Zebediah has pointed out [4] that it is not very elegant to (ab-)use
  the synchronization machinery for communicating the async result.

- Use APC_ASYNC_IO to perform the synchronous I/O [5].

  This ended up with a total of 11 patches, and turned out to be
  rather too complicated for a simple task.

- Add a new wineserver call, and use "add_queue" hook to save a round
  trip to the server [6].

</preamble>

Each of the above turned out to be either too intrusive, complicated, or
hard to verify.  Per suggestion by Zebediah [7], this is my (hopefully)
last approach: keep the new wineserver call, but keep out the implicit
STATUS_PENDING transition from "add_queue"; also, (re-)use the
"unknown_status" field.

Changelog:
- v1 -> v2: fix async queue hang on synchronous failure
- v2 -> v3: rewrite handling of pending/failed asyncs.
- v3 -> v4: don't set IOSB on synchronous failure, make more robust

[1] https://bugs.winehq.org/show_bug.cgi?id=52401
[2] https://www.winehq.org/pipermail/wine-devel/2021-May/186454.html
[3] https://www.winehq.org/pipermail/wine-devel/2022-January/204695.html
[4] https://www.winehq.org/pipermail/wine-devel/2022-January/204710.html
[5] https://www.winehq.org/pipermail/wine-devel/2022-January/205168.html
[6] https://www.winehq.org/pipermail/wine-devel/2022-January/205193.html
[7] https://www.winehq.org/pipermail/wine-devel/2022-January/205738.html

Jinoh Kang (5):
  server: Allow calling async_handoff() with status code STATUS_ALERTED.
  server: Add a new server request "notify_async_direct_result."
  server: Attempt to complete I/O request immediately in recv_socket.
  ntdll: Don't call try_recv before server call in sock_recv.
  server: Replace redundant recv_socket status fields with force_async
    boolean field.

 dlls/ntdll/unix/socket.c       | 37 ++++++++-------
 dlls/ntdll/unix/sync.c         | 22 +++++++++
 dlls/ntdll/unix/unix_private.h |  1 +
 dlls/ws2_32/tests/sock.c       |  8 ++--
 server/async.c                 | 86 ++++++++++++++++++++++++++++++++++
 server/protocol.def            | 14 +++++-
 server/sock.c                  | 40 +++++++++++-----
 7 files changed, 173 insertions(+), 35 deletions(-)

-- 
2.34.1

[RFC PATCH v4 1/5] server: Allow calling async_handoff() with status code STATUS_ALERTED.

[Jinoh Kang]

If the server detects that an I/O request could be completed immediately
(e.g. the socket to read from already has incoming data), it can now
return STATUS_ALERTED to allow opportunistic synchronous I/O.
The Unix side will then attempt to perform I/O in nonblocking mode and
report back the I/O status to the server with signal_wait_async().
If the operation returns e.g. EAGAIN or EWOULDBLOCK, the client can opt
to either abandon the request (by specifying an error status) or poll
for it in the server as usual (by waiting on the wait handle).

Without such mechanism in place, the client cannot safely perform
immediately satiable I/O operations synchronously, since it can
potentially conflict with other pending I/O operations that have already
been queued.

Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com>
---

Notes:
    v2 -> v3: set async->alerted = 1
    v3 -> v4: unchanged

 server/async.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/server/async.c b/server/async.c
index 7aef28355f0..e169bb23225 100644
--- a/server/async.c
+++ b/server/async.c
@@ -338,6 +338,30 @@ obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_
         return async->wait_handle;
     }
 
+    if (!async->pending && get_error() == STATUS_ALERTED)
+    {
+        /* Give the client opportunity to complete synchronously.
+         * If it turns out that the I/O request is not actually immediately satiable,
+         * the client may then choose to re-queue the async (with STATUS_PENDING).
+         */
+        async->unknown_status = 1;
+
+        /* Deactivate async so that it can be safely queued.
+         * Otherwise, the async will be erroneously alerted the next time
+         * someone calls async_wake_up() on the queue.
+         *
+         * Don't use async_terminate() here since we'd like to leave the IOSB
+         * status as STATUS_PENDING.  Also we certainly don't want APC_ASYNC_IO
+         * to fire in any circumstances.
+         */
+        async->terminated = 1;
+        async->alerted = 1;
+
+        async_reselect( async );
+
+        return async->wait_handle;
+    }
+
     async->initial_status = get_error();
 
     if (!async->pending && NT_ERROR( get_error() ))
-- 
2.34.1

[RFC PATCH v4 1/5] server: Allow calling async_handoff() with status code STATUS_ALERTED.

[Zebediah Figura]

On 1/29/22 01:47, Jinoh Kang wrote:
> If the server detects that an I/O request could be completed immediately
> (e.g. the socket to read from already has incoming data), it can now
> return STATUS_ALERTED to allow opportunistic synchronous I/O.
> The Unix side will then attempt to perform I/O in nonblocking mode and
> report back the I/O status to the server with signal_wait_async().
> If the operation returns e.g. EAGAIN or EWOULDBLOCK, the client can opt
> to either abandon the request (by specifying an error status) or poll
> for it in the server as usual (by waiting on the wait handle).
>
> Without such mechanism in place, the client cannot safely perform
> immediately satiable I/O operations synchronously, since it can
> potentially conflict with other pending I/O operations that have already
> been queued.
>
> Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com>
> ---
>
> Notes:
>      v2 -> v3: set async->alerted = 1
>      v3 -> v4: unchanged
>
> server/async.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
>
> diff --git a/server/async.c b/server/async.c
> index 7aef28355f0..e169bb23225 100644
> --- a/server/async.c
> +++ b/server/async.c
> @@ -338,6 +338,30 @@ obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_
>           return async->wait_handle;
>       }
>
> +    if (!async->pending && get_error() == STATUS_ALERTED)

Why "!async->pending"?
> +    {
> +        /* Give the client opportunity to complete synchronously.
> +         * If it turns out that the I/O request is not actually immediately satiable,
> +         * the client may then choose to re-queue the async (with STATUS_PENDING).

I'd explicitly mention *how* the async should be requeued here.
> +         */
> +        async->unknown_status = 1;
> +
> +        /* Deactivate async so that it can be safely queued.
> +         * Otherwise, the async will be erroneously alerted the next time
> +         * someone calls async_wake_up() on the queue.

Not to put too much weight on my own perspective, but I think it's 
clearer to explain *why* to do something, rather than why not. In this 
case, what we're doing is mimicking a normal "alert" case, except that 
we're not using an APC.
> +         *
> +         * Don't use async_terminate() here since we'd like to leave the IOSB
> +         * status as STATUS_PENDING.

Wait, why is this important?
> Also we certainly don't want APC_ASYNC_IO
> +         * to fire in any circumstances.
> +         */
> +        async->terminated = 1;
> +        async->alerted = 1;
> +
> +        async_reselect( async );
> +
> +        return async->wait_handle;
> +    }
> +
>       async->initial_status = get_error();
>
> if (!async->pending && NT_ERROR( get_error() ))

[RFC PATCH v4 1/5] server: Allow calling async_handoff() with status code STATUS_ALERTED.

[Jinoh Kang]

On 2/3/22 08:45, Zebediah Figura wrote:
> On 1/29/22 01:47, Jinoh Kang wrote:

...
> > diff --git a/server/async.c b/server/async.c
> > index 7aef28355f0..e169bb23225 100644
> > --- a/server/async.c
> > +++ b/server/async.c
> > @@ -338,6 +338,30 @@ obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_
> >           return async->wait_handle;
> >       }
> >   +    if (!async->pending && get_error() == STATUS_ALERTED)
>
> Why "!async->pending"?

The rationale is twofold:

1. async_handoff() can be called with "async->pending = 1".
   Notably the device manager and some sock IOCTLs call set_async_pending() before returning.
   Also, there exists another conditional statement below which explicitly tests for "async->pending".

2. If "async->pending && get_error() == STATUS_ALERTED", the async will be left in an inconsistent state.

   - There exists an established invariant that "async->unknown_status" implies "!async->pending".
     It would be invalid that "async->unknown_status && async->pending", since an async
     "even the initial state" of which "is not known yet" cannot possibly be queued as STATUS_PENDING.

   - Unless "async->direct_result", an APC_ASYNC_IO is required for the async to progress;
     Our code path however issues no APC calls, leaving the async in a stale, hard-to-debug state.

That said, an alternative would be to simply "assert( !async->pending )" inside the new code path.
However, I don't really see a reason to reject STATUS_ALERTED on a pending async; we can let it
simply follow the normal path, allowing the async to be terminated with STATUS_ALERTED as usual.
I think this best sticks to the principle of least surprise.  It's entirely my own perspective, though...
> > +    {
> > +        /* Give the client opportunity to complete synchronously.
> > +         * If it turns out that the I/O request is not actually immediately satiable,
> > +         * the client may then choose to re-queue the async (with STATUS_PENDING).
>
> I'd explicitly mention *how* the async should be requeued here.

I worded it that way since each I/O operation can have their own way to manage their async queue
(e.g. socket operations would use sock->read_q/write_q, device managers would use its own IRP queue,
while others may just use fd->read_q/write_q).  I chose to retain the abstractness so that the comment would't get easily stale or out of date.

That said, I suppose you specifically meant mentioning how to _transition_ the async to STATUS_PENDING.
I agree with you in that case.  I'll add a mention of the new wineserver call here.
> > +         */
> > +        async->unknown_status = 1;
> > +
> > +        /* Deactivate async so that it can be safely queued.
> > +         * Otherwise, the async will be erroneously alerted the next time
> > +         * someone calls async_wake_up() on the queue.
>
> Not to put too much weight on my own perspective, but I think it's clearer to explain *why* to do something, rather than why not.

By "why not" did you meant the "otherwise" part?
> In this case, what we're doing is mimicking a normal "alert" case, except that we're not using an APC.

If I understood correctly, I should just say instead:
> Emulate calling async_terminate() with STATUS_ALERTED (request client to complete I/O),
> but don't actually queue APC_ASYNC_IO under any circumstances.
> Is this correct?
> > +         *
> > +         * Don't use async_terminate() here since we'd like to leave the IOSB
> > +         * status as STATUS_PENDING. 
>
> Wait, why is this important?

Oops, looks like a leftover from v3.  My apologies...
(In v3 it was required for making async_handoff() behave on async "restart" case and properly return STATUS_PENDING; otherwise, it would notice the STATUS_ALERTED in async->iosb->status and return just that, ignoring our request to set the status to STATUS_PENDING and restart the async.)
> > Also we certainly don't want APC_ASYNC_IO
> > +         * to fire in any circumstances.
> > +         */
> > +        async->terminated = 1;
> > +        async->alerted = 1;
> > +
> > +        async_reselect( async );
> > +
> > +        return async->wait_handle;
> > +    }
> > +
> >       async->initial_status = get_error();
> >         if (!async->pending && NT_ERROR( get_error() ))

-- 
Sincerely,
Jinoh Kang

[RFC PATCH v4 1/5] server: Allow calling async_handoff() with status code STATUS_ALERTED.

[Zebediah Figura]

On 2/3/22 09:28, Jinoh Kang wrote:
> On 2/3/22 08:45, Zebediah Figura wrote:
> > On 1/29/22 01:47, Jinoh Kang wrote:
>
> ...
> > > diff --git a/server/async.c b/server/async.c
> > > index 7aef28355f0..e169bb23225 100644
> > > --- a/server/async.c
> > > +++ b/server/async.c
> > > @@ -338,6 +338,30 @@ obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_
> > >            return async->wait_handle;
> > >        }
> > >    +    if (!async->pending && get_error() == STATUS_ALERTED)
> >
> > Why "!async->pending"?
>
> The rationale is twofold:
>
> 1. async_handoff() can be called with "async->pending = 1".
>     Notably the device manager and some sock IOCTLs call set_async_pending() before returning.
>     Also, there exists another conditional statement below which explicitly tests for "async->pending".

Yes, but they don't set the last error to STATUS_ALERTED, do they?

Anything that's triggered below should only apply to asyncs whose 
initial status is known. Consider that "unknown_status" not only means 
the initial NTSTATUS is unknown, but it also means the initial pending 
state is unknown.
> 2. If "async->pending && get_error() == STATUS_ALERTED", the async will be left in an inconsistent state.
>
> - There exists an established invariant that "async->unknown_status" implies "!async->pending".
>       It would be invalid that "async->unknown_status && async->pending", since an async
>       "even the initial state" of which "is not known yet" cannot possibly be queued as STATUS_PENDING.
>
> - Unless "async->direct_result", an APC_ASYNC_IO is required for the async to progress;
>       Our code path however issues no APC calls, leaving the async in a stale, hard-to-debug state.
>
> That said, an alternative would be to simply "assert( !async->pending )" inside the new code path.
> However, I don't really see a reason to reject STATUS_ALERTED on a pending async; we can let it
> simply follow the normal path, allowing the async to be terminated with STATUS_ALERTED as usual.
> I think this best sticks to the principle of least surprise.  It's entirely my own perspective, though...

If you consider the above, "async->pending" is semantically undefined if 
async->unknown_status is 1. We shouldn't depend on it being one thing or 
the other, and checking for that (even in an assert) I think gives the 
wrong idea that we care.
> > > +    {
> > > +        /* Give the client opportunity to complete synchronously.
> > > +         * If it turns out that the I/O request is not actually immediately satiable,
> > > +         * the client may then choose to re-queue the async (with STATUS_PENDING).
> >
> > I'd explicitly mention *how* the async should be requeued here.
>
> I worded it that way since each I/O operation can have their own way to manage their async queue
> (e.g. socket operations would use sock->read_q/write_q, device managers would use its own IRP queue,
> while others may just use fd->read_q/write_q).  I chose to retain the abstractness so that the comment would't get easily stale or out of date.
>
> That said, I suppose you specifically meant mentioning how to _transition_ the async to STATUS_PENDING.
> I agree with you in that case.  I'll add a mention of the new wineserver call here.

Yeah, that's what I meant :-)
> > > +         */
> > > +        async->unknown_status = 1;
> > > +
> > > +        /* Deactivate async so that it can be safely queued.
> > > +         * Otherwise, the async will be erroneously alerted the next time
> > > +         * someone calls async_wake_up() on the queue.
> >
> > Not to put too much weight on my own perspective, but I think it's clearer to explain *why* to do something, rather than why not.
>
> By "why not" did you meant the "otherwise" part?
> > In this case, what we're doing is mimicking a normal "alert" case, except that we're not using an APC.
>
> If I understood correctly, I should just say instead:
> > Emulate calling async_terminate() with STATUS_ALERTED (request client to complete I/O),
> > but don't actually queue APC_ASYNC_IO under any circumstances.
> > Is this correct?

Something more like that, yeah. Although, to contradict my earlier 
assertion, I think it'd also help to explain what's wrong with using 
APC_ASYNC_IO here.

[RFC PATCH v4 1/5] server: Allow calling async_handoff() with status code STATUS_ALERTED.

[Jinoh Kang]

On 2/4/22 03:20, Zebediah Figura wrote:
> On 2/3/22 09:28, Jinoh Kang wrote:
> > On 2/3/22 08:45, Zebediah Figura wrote:
> > > On 1/29/22 01:47, Jinoh Kang wrote:
> >
> > ...
> > > > diff --git a/server/async.c b/server/async.c
> > > > index 7aef28355f0..e169bb23225 100644
> > > > --- a/server/async.c
> > > > +++ b/server/async.c
> > > > @@ -338,6 +338,30 @@ obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_
> > > >            return async->wait_handle;
> > > >        }
> > > >    +    if (!async->pending && get_error() == STATUS_ALERTED)
> > >
> > > Why "!async->pending"?
> >
> > The rationale is twofold:
> >
> > 1. async_handoff() can be called with "async->pending = 1".
> >     Notably the device manager and some sock IOCTLs call set_async_pending() before returning.
> >     Also, there exists another conditional statement below which explicitly tests for "async->pending".
>
> Yes, but they don't set the last error to STATUS_ALERTED, do they?

They don't (yet), hence the idea of "assert()".
I suppose it's better to quit early so it'd be easiler to debug if STATUS_ALERTED _somehow_ finds its way to sneak in and screw up everything due to missing APC...
> Anything that's triggered below should only apply to asyncs whose initial status is known. Consider that "unknown_status" not only means the initial NTSTATUS is unknown, but it also means the initial pending state is unknown.

It appears that existing code dictates that the initial pending state of unknown_status async is, well, "not pending."  More on it below.
> > 2. If "async->pending && get_error() == STATUS_ALERTED", the async will be left in an inconsistent state.
> >
> >     - There exists an established invariant that "async->unknown_status" implies "!async->pending".
> >       It would be invalid that "async->unknown_status && async->pending", since an async
> >       "even the initial state" of which "is not known yet" cannot possibly be queued as STATUS_PENDING.
> >
> >     - Unless "async->direct_result", an APC_ASYNC_IO is required for the async to progress;
> >       Our code path however issues no APC calls, leaving the async in a stale, hard-to-debug state.
> >
> > That said, an alternative would be to simply "assert( !async->pending )" inside the new code path.
> > However, I don't really see a reason to reject STATUS_ALERTED on a pending async; we can let it
> > simply follow the normal path, allowing the async to be terminated with STATUS_ALERTED as usual.
> > I think this best sticks to the principle of least surprise.  It's entirely my own perspective, though...
>
> If you consider the above, "async->pending" is semantically undefined if async->unknown_status is 1. We shouldn't depend on it being one thing or the other, and checking for that (even in an assert) I think gives the wrong idea that we care.

On the contrary, there are a total of three places in async.c that do rely on the fact that "async->unknown_status" implies "!async->pending".
For example, note the following code in async_set_result():
> /* don't signal completion if the async failed synchronously
>         * this can happen if the initial status was unknown (i.e. for device files)
>         * note that we check the IOSB status here, not the initial status */
>        if (async->pending || !NT_ERROR( status ))
>        {
>            if (async->data.apc)
>            {
>                apc_call_t data;
>                memset( &data, 0, sizeof(data) );
>                data.type         = APC_USER;
>                data.user.func    = async->data.apc;
>                data.user.args[0] = async->data.apc_context;
>                data.user.args[1] = async->data.iosb;
>                data.user.args[2] = 0;
>                thread_queue_apc( NULL, async->thread, NULL, &data );
>            }
>            else if (async->data.apc_context && (async->pending ||
>                     !(async->comp_flags & FILE_SKIP_COMPLETION_PORT_ON_SUCCESS)))
>            {
>                add_async_completion( async, async->data.apc_context, status, total );
>            }
>
> if (async->event) set_event( async->event );
>            else if (async->fd) set_fd_signaled( async->fd, 1 );
>        }

There's another one in async_terminate():
> /* this can happen if the initial status was unknown (i.e. for device
>         * files). the client should not fill the IOSB in this case; pass it as
>         * NULL to communicate that.
>         * note that we check the IOSB status and not the initial status */
>        if (NT_ERROR( status ) && (!is_fd_overlapped( async->fd ) || !async->pending))
>            data.async_io.sb = 0;
>        else
>            data.async_io.sb = async->data.iosb;

(The other one is in async_handoff, but the check is not done if unknown_status is set anyway.)

Notice that they are using "!async->pending" as a covering condition for "async->unknown_status".
So I'd argue it's not semantically undefined at all; the semantics of "unknown_status" actually _entails_ that the async is not "pending".
It turns out this plays a significant role in the lifecycle of IRP-based asyncs.  Assuming the device file is not opened for synchronous I/O, the process is as follows:

1. Initially, queue_irp() marks the IRP async as having unknown status.

2. async_handoff() sees that the unknown_status flag is set, and keeps the wait handle open. The wait handle returned to the client.

3. The client starts to block on the wait handle.

4. Later, the driver signals the initial status of the IRP as STATUS_PENDING (the IRP has been queued internally by the driver).

5. The get_next_device_request handler sets the initial status and pending state of async, and wakes up the client.

6. The client stops waiting and returns the status, which is possibly STATUS_PENDING.

In steps 1-4, the async stays in the "not pending" state.  Had the async been in "pending" state and the I/O failed, step 5 would erroneously trigger APC / IOCP notification mechanism since the failure is treated as asynchronous instead of synchronous.

In this regard, I would argue that the pending flag is best documented as "has the async entered the asynchronous phrase, so that the status would be reported to the client even if it failed?".

In conclusion, we depend on it and we should care.
> > > > +    {
> > > > +        /* Give the client opportunity to complete synchronously.
> > > > +         * If it turns out that the I/O request is not actually immediately satiable,
> > > > +         * the client may then choose to re-queue the async (with STATUS_PENDING).
> > >
> > > I'd explicitly mention *how* the async should be requeued here.
> >
> > I worded it that way since each I/O operation can have their own way to manage their async queue
> > (e.g. socket operations would use sock->read_q/write_q, device managers would use its own IRP queue,
> > while others may just use fd->read_q/write_q).  I chose to retain the abstractness so that the comment would't get easily stale or out of date.
> >
> > That said, I suppose you specifically meant mentioning how to _transition_ the async to STATUS_PENDING.
> > I agree with you in that case.  I'll add a mention of the new wineserver call here.
>
> Yeah, that's what I meant :-)
> > > > +         */
> > > > +        async->unknown_status = 1;
> > > > +
> > > > +        /* Deactivate async so that it can be safely queued.
> > > > +         * Otherwise, the async will be erroneously alerted the next time
> > > > +         * someone calls async_wake_up() on the queue.
> > >
> > > Not to put too much weight on my own perspective, but I think it's clearer to explain *why* to do something, rather than why not.
> >
> > By "why not" did you meant the "otherwise" part?
> > > In this case, what we're doing is mimicking a normal "alert" case, except that we're not using an APC.
> >
> > If I understood correctly, I should just say instead:
> > > Emulate calling async_terminate() with STATUS_ALERTED (request client to complete I/O),
> > > but don't actually queue APC_ASYNC_IO under any circumstances.
> > > Is this correct?
>
> Something more like that, yeah. Although, to contradict my earlier assertion, I think it'd also help to explain what's wrong with using APC_ASYNC_IO here.

-- 
Sincerely,
Jinoh Kang

[RFC PATCH v4 1/5] server: Allow calling async_handoff() with status code STATUS_ALERTED.

[Jinoh Kang]

On 2/4/22 14:50, Jinoh Kang wrote:
> Notice that they are using "!async->pending" as a covering condition for "async->unknown_status".
> So I'd argue it's not semantically undefined at all; the semantics of "unknown_status" actually _entails_ that the async is not "pending".
> It turns out this plays a significant role in the lifecycle of IRP-based asyncs.  Assuming the device file is not opened for synchronous I/O, the process is as follows:
>
> 1. Initially, queue_irp() marks the IRP async as having unknown status.
>
> 2. async_handoff() sees that the unknown_status flag is set, and keeps the wait handle open. The wait handle returned to the client.
>
> 3. The client starts to block on the wait handle.
>
> 4. Later, the driver signals the initial status of the IRP as STATUS_PENDING (the IRP has been queued internally by the driver).
>
> 5. The get_next_device_request handler sets the initial status and pending state of async, and wakes up the client.
>
> 6. The client stops waiting and returns the status, which is possibly STATUS_PENDING.
>
> In steps 1-4, the async stays in the "not pending" state.  Had the async been in "pending" state and the I/O failed,

correction: and the I/O failed (step 4 set an error code instead of STATUS_PENDING),
> step 5 would erroneously trigger APC / IOCP notification mechanism since the failure is treated as asynchronous instead of synchronous.
>
> In this regard, I would argue that the pending flag is best documented as "has the async entered the asynchronous phrase, so that the status would be reported to the client even if it failed?".
>
> In conclusion, we depend on it and we should care.

-- 
Sincerely,
Jinoh Kang

[RFC PATCH v4 1/5] server: Allow calling async_handoff() with status code STATUS_ALERTED.

[Zebediah Figura]

On 2/3/22 23:50, Jinoh Kang wrote:
> On 2/4/22 03:20, Zebediah Figura wrote:
> > On 2/3/22 09:28, Jinoh Kang wrote:
> > > On 2/3/22 08:45, Zebediah Figura wrote:
> > > > On 1/29/22 01:47, Jinoh Kang wrote:
> > >
> > > ...
> > > > > diff --git a/server/async.c b/server/async.c
> > > > > index 7aef28355f0..e169bb23225 100644
> > > > > --- a/server/async.c
> > > > > +++ b/server/async.c
> > > > > @@ -338,6 +338,30 @@ obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_
> > > > >             return async->wait_handle;
> > > > >         }
> > > > >     +    if (!async->pending && get_error() == STATUS_ALERTED)
> > > >
> > > > Why "!async->pending"?
> > >
> > > The rationale is twofold:
> > >
> > > 1. async_handoff() can be called with "async->pending = 1".
> > >      Notably the device manager and some sock IOCTLs call set_async_pending() before returning.
> > >      Also, there exists another conditional statement below which explicitly tests for "async->pending".
> >
> > Yes, but they don't set the last error to STATUS_ALERTED, do they?
>
> They don't (yet), hence the idea of "assert()".
> I suppose it's better to quit early so it'd be easiler to debug if STATUS_ALERTED _somehow_ finds its way to sneak in and screw up everything due to missing APC...
> > Anything that's triggered below should only apply to asyncs whose initial status is known. Consider that "unknown_status" not only means the initial NTSTATUS is unknown, but it also means the initial pending state is unknown.
>
> It appears that existing code dictates that the initial pending state of unknown_status async is, well, "not pending."  More on it below.
> > > 2. If "async->pending && get_error() == STATUS_ALERTED", the async will be left in an inconsistent state.
> > >
> > >     - There exists an established invariant that "async->unknown_status" implies "!async->pending".
> > >        It would be invalid that "async->unknown_status && async->pending", since an async
> > >        "even the initial state" of which "is not known yet" cannot possibly be queued as STATUS_PENDING.
> > >
> > >     - Unless "async->direct_result", an APC_ASYNC_IO is required for the async to progress;
> > >        Our code path however issues no APC calls, leaving the async in a stale, hard-to-debug state.
> > >
> > > That said, an alternative would be to simply "assert( !async->pending )" inside the new code path.
> > > However, I don't really see a reason to reject STATUS_ALERTED on a pending async; we can let it
> > > simply follow the normal path, allowing the async to be terminated with STATUS_ALERTED as usual.
> > > I think this best sticks to the principle of least surprise.  It's entirely my own perspective, though...
> >
> > If you consider the above, "async->pending" is semantically undefined if async->unknown_status is 1. We shouldn't depend on it being one thing or the other, and checking for that (even in an assert) I think gives the wrong idea that we care.
>
> On the contrary, there are a total of three places in async.c that do rely on the fact that "async->unknown_status" implies "!async->pending".
> For example, note the following code in async_set_result():
> > /* don't signal completion if the async failed synchronously
> >          * this can happen if the initial status was unknown (i.e. for device files)
> >          * note that we check the IOSB status here, not the initial status */
> >         if (async->pending || !NT_ERROR( status ))
> >         {
> >             if (async->data.apc)
> >             {
> >                 apc_call_t data;
> >                 memset( &data, 0, sizeof(data) );
> >                 data.type         = APC_USER;
> >                 data.user.func    = async->data.apc;
> >                 data.user.args[0] = async->data.apc_context;
> >                 data.user.args[1] = async->data.iosb;
> >                 data.user.args[2] = 0;
> >                 thread_queue_apc( NULL, async->thread, NULL, &data );
> >             }
> >             else if (async->data.apc_context && (async->pending ||
> >                      !(async->comp_flags & FILE_SKIP_COMPLETION_PORT_ON_SUCCESS)))
> >             {
> >                 add_async_completion( async, async->data.apc_context, status, total );
> >             }
> >
> > if (async->event) set_event( async->event );
> >             else if (async->fd) set_fd_signaled( async->fd, 1 );
> >         }

Well, no, not really. Note that async->unknown_status actually has to be 
zero here. The point is that *usually* we won't get into 
async_set_result() if (!async->pending && NT_ERROR( status )), because, 
well, we don't need to. The server request (read, write, ioctl, 
whatever) will return failure and no async handle, because we don't need 
to fill the IOSB or signal completion in any way.

Note also that "was async->unknown_status ever true?" is also completely 
orthogonal of "is async->pending currently true?". The latter is set 
according to whether the IRP was marked pending via IoMarkIrpPending(), 
and it's supposed to have the exact same semantics.
> There's another one in async_terminate():
> > /* this can happen if the initial status was unknown (i.e. for device
> >          * files). the client should not fill the IOSB in this case; pass it as
> >          * NULL to communicate that.
> >          * note that we check the IOSB status and not the initial status */
> >         if (NT_ERROR( status ) && (!is_fd_overlapped( async->fd ) || !async->pending))
> >             data.async_io.sb = 0;
> >         else
> >             data.async_io.sb = async->data.iosb;

Same deal here.
> (The other one is in async_handoff, but the check is not done if unknown_status is set anyway.)
>
> Notice that they are using "!async->pending" as a covering condition for "async->unknown_status".
> So I'd argue it's not semantically undefined at all; the semantics of "unknown_status" actually _entails_ that the async is not "pending".
> It turns out this plays a significant role in the lifecycle of IRP-based asyncs.  Assuming the device file is not opened for synchronous I/O, the process is as follows:
>
> 1. Initially, queue_irp() marks the IRP async as having unknown status.
>
> 2. async_handoff() sees that the unknown_status flag is set, and keeps the wait handle open. The wait handle returned to the client.
>
> 3. The client starts to block on the wait handle.
>
> 4. Later, the driver signals the initial status of the IRP as STATUS_PENDING (the IRP has been queued internally by the driver).
>
> 5. The get_next_device_request handler sets the initial status and pending state of async, and wakes up the client.
>
> 6. The client stops waiting and returns the status, which is possibly STATUS_PENDING.
>
> In steps 1-4, the async stays in the "not pending" state.  Had the async been in "pending" state and the I/O failed, step 5 would erroneously trigger APC / IOCP notification mechanism since the failure is treated as asynchronous instead of synchronous.
>
> In this regard, I would argue that the pending flag is best documented as "has the async entered the asynchronous phrase, so that the status would be reported to the client even if it failed?".
>
> In conclusion, we depend on it and we should care.

Except that I don't think that's how you should be thinking about 
async->pending. async->pending is meant to answer the question "will we 
fill the IOSB and signal completion even if the async fails?" If 
async->unknown_status, the answer to that is "how should I know? we 
haven't got that far yet".

The only times we ever care, or should care, about that flag, are when 
we're determining whether (or sometimes, as a consequence, how) to queue 
completion. We shouldn't care about it if the initial status isn't even 
known yet.

[RFC PATCH v4 1/5] server: Allow calling async_handoff() with status code STATUS_ALERTED.

[Jinoh Kang]

On 2/5/22 02:55, Zebediah Figura wrote:
> On 2/3/22 23:50, Jinoh Kang wrote:
> > On 2/4/22 03:20, Zebediah Figura wrote:
> > > On 2/3/22 09:28, Jinoh Kang wrote:
> > > > On 2/3/22 08:45, Zebediah Figura wrote:
> > > > > On 1/29/22 01:47, Jinoh Kang wrote:
> > > >
> > > > ...
> > > > > > diff --git a/server/async.c b/server/async.c
> > > > > > index 7aef28355f0..e169bb23225 100644
> > > > > > --- a/server/async.c
> > > > > > +++ b/server/async.c
> > > > > > @@ -338,6 +338,30 @@ obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_
> > > > > >             return async->wait_handle;
> > > > > >         }
> > > > > >     +    if (!async->pending && get_error() == STATUS_ALERTED)
> > > > >
> > > > > Why "!async->pending"?
> > > >
> > > > The rationale is twofold:
> > > >
> > > > 1. async_handoff() can be called with "async->pending = 1".
> > > >      Notably the device manager and some sock IOCTLs call set_async_pending() before returning.
> > > >      Also, there exists another conditional statement below which explicitly tests for "async->pending".
> > >
> > > Yes, but they don't set the last error to STATUS_ALERTED, do they?
> >
> > They don't (yet), hence the idea of "assert()".
> > I suppose it's better to quit early so it'd be easiler to debug if STATUS_ALERTED _somehow_ finds its way to sneak in and screw up everything due to missing APC...
> > > Anything that's triggered below should only apply to asyncs whose initial status is known. Consider that "unknown_status" not only means the initial NTSTATUS is unknown, but it also means the initial pending state is unknown.
> >
> > It appears that existing code dictates that the initial pending state of unknown_status async is, well, "not pending."  More on it below.
> > > > 2. If "async->pending && get_error() == STATUS_ALERTED", the async will be left in an inconsistent state.
> > > >
> > > >      - There exists an established invariant that "async->unknown_status" implies "!async->pending".
> > > >        It would be invalid that "async->unknown_status && async->pending", since an async
> > > >        "even the initial state" of which "is not known yet" cannot possibly be queued as STATUS_PENDING.
> > > >
> > > >      - Unless "async->direct_result", an APC_ASYNC_IO is required for the async to progress;
> > > >        Our code path however issues no APC calls, leaving the async in a stale, hard-to-debug state.
> > > >
> > > > That said, an alternative would be to simply "assert( !async->pending )" inside the new code path.
> > > > However, I don't really see a reason to reject STATUS_ALERTED on a pending async; we can let it
> > > > simply follow the normal path, allowing the async to be terminated with STATUS_ALERTED as usual.
> > > > I think this best sticks to the principle of least surprise.  It's entirely my own perspective, though...
> > >
> > > If you consider the above, "async->pending" is semantically undefined if async->unknown_status is 1. We shouldn't depend on it being one thing or the other, and checking for that (even in an assert) I think gives the wrong idea that we care.
> >
> > On the contrary, there are a total of three places in async.c that do rely on the fact that "async->unknown_status" implies "!async->pending".
> > For example, note the following code in async_set_result():
> > >         /* don't signal completion if the async failed synchronously
> > >          * this can happen if the initial status was unknown (i.e. for device files)
> > >          * note that we check the IOSB status here, not the initial status */
> > >         if (async->pending || !NT_ERROR( status ))
> > >         {
> > >             if (async->data.apc)
> > >             {
> > >                 apc_call_t data;
> > >                 memset( &data, 0, sizeof(data) );
> > >                 data.type         = APC_USER;
> > >                 data.user.func    = async->data.apc;
> > >                 data.user.args[0] = async->data.apc_context;
> > >                 data.user.args[1] = async->data.iosb;
> > >                 data.user.args[2] = 0;
> > >                 thread_queue_apc( NULL, async->thread, NULL, &data );
> > >             }
> > >             else if (async->data.apc_context && (async->pending ||
> > >                      !(async->comp_flags & FILE_SKIP_COMPLETION_PORT_ON_SUCCESS)))
> > >             {
> > >                 add_async_completion( async, async->data.apc_context, status, total );
> > >             }
> > >
> > >             if (async->event) set_event( async->event );
> > >             else if (async->fd) set_fd_signaled( async->fd, 1 );
> > >         }
>
> Well, no, not really. Note that async->unknown_status actually has to be zero here.

Oops.  That alone invalidates my point.  Sorry for the noise...

Now that we don't care about async->pending, I changed the async_handoff patch to read:
> if (get_error() == STATUS_ALERTED)
>    {
>        /* Give the client opportunity to complete synchronously.  After the
>         * client performs the I/O, it reports the result back to the server via
>         * the set_async_direct_result server call.  If it turns out that the
>         * I/O request is not actually immediately satiable, the client may then
>         * choose to re-queue the async by reporting STATUS_PENDING instead.
>         *
>         * Since we're deferring the initial I/O to the client, we mark the
>         * async as having an unknown initial status (unknown_status). Unlike
>         * async_set_unknown_status() however, we leave direct_result unset.
>         * This is because we wouldn't normally want APC_ASYNC_IO to be queued
>         * to the server, since the semantics of synchronous vs. asynchronous
>         * completion are subtlely different.
>         */
>        async->unknown_status = 1;
>        async_terminate( async, STATUS_ALERTED );
>        return async->wait_handle;
>    }

Note the call to async_terminate().  We 1. shouldn't care whatever status is in the IOSB,
and 2. the async_handoff() is expected to be called with "async->direct_result = 0" anyway.
So I decided that there shouldn't be any problem with using async_terminate() as-is,
and I also personally think the code is clearer that way.  Is this okay with you
(and whoever else is reading this)?
> The point is that *usually* we won't get into async_set_result() if (!async->pending && NT_ERROR( status )), because, well, we don't need to. The server request (read, write, ioctl, whatever) will return failure and no async handle, because we don't need to fill the IOSB or signal completion in any way.
>
> Note also that "was async->unknown_status ever true?" is also completely orthogonal of "is async->pending currently true?". The latter is set according to whether the IRP was marked pending via IoMarkIrpPending(), and it's supposed to have the exact same semantics.
> > There's another one in async_terminate():
> > >         /* this can happen if the initial status was unknown (i.e. for device
> > >          * files). the client should not fill the IOSB in this case; pass it as
> > >          * NULL to communicate that.
> > >          * note that we check the IOSB status and not the initial status */
> > >         if (NT_ERROR( status ) && (!is_fd_overlapped( async->fd ) || !async->pending))
> > >             data.async_io.sb = 0;
> > >         else
> > >             data.async_io.sb = async->data.iosb;
>
> Same deal here.
> > (The other one is in async_handoff, but the check is not done if unknown_status is set anyway.)
> >
> > Notice that they are using "!async->pending" as a covering condition for "async->unknown_status".
> > So I'd argue it's not semantically undefined at all; the semantics of "unknown_status" actually _entails_ that the async is not "pending".
> > It turns out this plays a significant role in the lifecycle of IRP-based asyncs.  Assuming the device file is not opened for synchronous I/O, the process is as follows:
> >
> > 1. Initially, queue_irp() marks the IRP async as having unknown status.
> >
> > 2. async_handoff() sees that the unknown_status flag is set, and keeps the wait handle open. The wait handle returned to the client.
> >
> > 3. The client starts to block on the wait handle.
> >
> > 4. Later, the driver signals the initial status of the IRP as STATUS_PENDING (the IRP has been queued internally by the driver).
> >
> > 5. The get_next_device_request handler sets the initial status and pending state of async, and wakes up the client.
> >
> > 6. The client stops waiting and returns the status, which is possibly STATUS_PENDING.
> >
> > In steps 1-4, the async stays in the "not pending" state.  Had the async been in "pending" state and the I/O failed, step 5 would erroneously trigger APC / IOCP notification mechanism since the failure is treated as asynchronous instead of synchronous.
> >
> > In this regard, I would argue that the pending flag is best documented as "has the async entered the asynchronous phrase, so that the status would be reported to the client even if it failed?".
> >
> > In conclusion, we depend on it and we should care.
>
> Except that I don't think that's how you should be thinking about async->pending. async->pending is meant to answer the question "will we fill the IOSB and signal completion even if the async fails?" If async->unknown_status, the answer to that is "how should I know? we haven't got that far yet".
>
> The only times we ever care, or should care, about that flag, are when we're determining whether (or sometimes, as a consequence, how) to queue completion. We shouldn't care about it if the initial status isn't even known yet.

-- 
Sincerely,
Jinoh Kang

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

[Jinoh Kang]

Some I/O operations need a way to "queue" the async to the target object
first, even if the operation itself is to be completed synchronously.
After synchronous completion, it needs a way to notify the
IO_STATUS_BLOCK values back to the server.

Add a new wineserver request, "notify_async_direct_result", which
notifies direct (i.e. synchronous) completion of async from the same
thread.

Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com>
---

Notes:
    v1 -> v2: dequeue async after failed completion
    v2 -> v3: rewrite handling of pending/failed asyncs.
    v3 -> v4:
    - add assert( async->iosb );
    - make robust against abrupt transition/cancellation:
      don't return STATUS_ACCESS_DENIED on precondition failure
      (client won't know what to do with STATUS_ACCESS_DENIED anyway)
    - always return wait_handle if possible
    - clarify and rearrange comments

 server/async.c      | 62 +++++++++++++++++++++++++++++++++++++++++++++
 server/protocol.def | 10 ++++++++
 2 files changed, 72 insertions(+)

diff --git a/server/async.c b/server/async.c
index e169bb23225..8852ff1c6d5 100644
--- a/server/async.c
+++ b/server/async.c
@@ -752,3 +752,65 @@ DECL_HANDLER(get_async_result)
     }
     set_error( iosb->status );
 }
+
+/* Notify direct completion of async and close the wait handle if not blocking */
+DECL_HANDLER(notify_async_direct_result)
+{
+    struct async *async = (struct async *)get_handle_obj( current->process, req->handle, 0, &async_ops );
+
+    if (!async) return;
+
+    /* we only return an async handle for asyncs created via create_request_async() */
+    assert( async->iosb );
+
+    if (async->unknown_status && !async->pending &&
+        async->terminated && async->alerted &&
+        async->iosb->status == STATUS_PENDING)
+    {
+        /* async->terminated is 1, so async_terminate() won't do anything.
+         * We need to set the status and result for ourselves.
+         */
+        async->iosb->status = req->status;
+        async->iosb->result = req->information;
+
+        /* Set status for async_handoff(). */
+        set_error( async->iosb->status );
+
+        /* 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.
+         *
+         * NOTE: async_handoff() is being called with async->terminated = 1.
+         */
+        async->unknown_status = 0;
+        async_handoff( async, NULL, 0 );
+
+        /* If the I/O has completed successfully, the client would have already
+         * set the IOSB. Therefore, we can skip waiting on wait_handle and do
+         * async_set_result() directly.
+         */
+        async_set_result( &async->obj, async->iosb->status, async->iosb->result );
+
+        /* Close wait handle here to avoid extra server round trip if the I/O
+         * has completed successfully.
+         *
+         * - If STATUS_PENDING, async_handoff() decides whether to close
+         *   the wait handle for us.  The wait handle is kept open if
+         *   async->blocking is set (i.e. we're going to block on it).
+         *
+         * - If failed (NT_ERROR), async_handoff() always closes
+         *   the wait_handle, which means async->wait_handle == 0.
+         */
+        if (async->wait_handle && async->iosb->status != STATUS_PENDING)
+        {
+            close_handle( async->thread->process, async->wait_handle );
+            async->wait_handle = 0;
+        }
+    } /* Otherwise, async have been abruptly transitioned (cancelled?). */
+
+    /* async_set_result() may have overriden the error value.  Reset it. */
+    set_error( async->iosb->status );
+    reply->handle = async->wait_handle;
+
+    release_object( &async->obj );
+}
diff --git a/server/protocol.def b/server/protocol.def
index 02e73047f9b..d79eca074a0 100644
--- a/server/protocol.def
+++ b/server/protocol.def
@@ -2163,6 +2163,16 @@ enum message_type
 @END
 
 
+/* Notify direct completion of async and close the wait handle if not blocking */
+(a)REQ(notify_async_direct_result)
+    obj_handle_t   handle;        /* wait handle */
+    unsigned int   status;        /* completion status */
+    apc_param_t    information;   /* IO_STATUS_BLOCK Information */
+(a)REPLY
+    obj_handle_t   handle;        /* wait handle, or NULL if closed */
+(a)END
+
+
 /* Perform a read on a file object */
 @REQ(read)
     async_data_t   async;         /* async I/O parameters */
-- 
2.34.1

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

[Zebediah Figura]

On 1/29/22 01:47, Jinoh Kang wrote:
> Some I/O operations need a way to "queue" the async to the target object
> first, even if the operation itself is to be completed synchronously.
> After synchronous completion, it needs a way to notify the
> IO_STATUS_BLOCK values back to the server.
>
> Add a new wineserver request, "notify_async_direct_result", which
> notifies direct (i.e. synchronous) completion of async from the same
> thread.
>
> Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com>
> ---
>
> Notes:
>      v1 -> v2: dequeue async after failed completion
>      v2 -> v3: rewrite handling of pending/failed asyncs.
>      v3 -> v4:
>      - add assert( async->iosb );
>      - make robust against abrupt transition/cancellation:
>        don't return STATUS_ACCESS_DENIED on precondition failure
>        (client won't know what to do with STATUS_ACCESS_DENIED anyway)
>      - always return wait_handle if possible
>      - clarify and rearrange comments
>
> server/async.c      | 62 +++++++++++++++++++++++++++++++++++++++++++++
>   server/protocol.def | 10 ++++++++
>   2 files changed, 72 insertions(+)
>
> diff --git a/server/async.c b/server/async.c
> index e169bb23225..8852ff1c6d5 100644
> --- a/server/async.c
> +++ b/server/async.c
> @@ -752,3 +752,65 @@ DECL_HANDLER(get_async_result)
>       }
>       set_error( iosb->status );
>   }
> +
> +/* Notify direct completion of async and close the wait handle if not blocking */
> +DECL_HANDLER(notify_async_direct_result)
> +{
> +    struct async *async = (struct async *)get_handle_obj( current->process, req->handle, 0, &async_ops );
> +
> +    if (!async) return;
> +
> +    /* we only return an async handle for asyncs created via create_request_async() */
> +    assert( async->iosb );
> +
> +    if (async->unknown_status && !async->pending &&
> +        async->terminated && async->alerted &&
> +        async->iosb->status == STATUS_PENDING)

So, these are all things that happen to be true assuming the client is 
behaving correctly, but the way this is written doesn't exactly make 
that clear (in particular, with a large if block, I can't see what the 
"else" case looks like immediately). I would recommend inverting the 
condition, returning early, and possibly returning some error condition 
(STATUS_INVALID_PARAMETER?) to make it clear that the client is misbehaving.

Note also that "async->pending" is kind of irrelevant here, it 
semantically is only meaningful when considering things like whether the 
request should queue completion. I.e. it can kind of be considered 
undefined if async->unknown_status == 1.

I'm not sure async->iosb->status is meaningful either, is it? We check 
for "iosb->status == STATUS_PENDING" in a few places, but I'm not really 
sure that's a good idea; we should probably be treating it more like 
"the value that gets returned in the userspace IOSB". In any case I 
don't think it has semantic value here.
> +    {
> +        /* async->terminated is 1, so async_terminate() won't do anything.
> +         * We need to set the status and result for ourselves.
> +         */
> +        async->iosb->status = req->status;
> +        async->iosb->result = req->information;

Except that neither of these variables actually matter past 
async_set_result(), right? We shouldn't need to bother.
> +
> +        /* Set status for async_handoff(). */
> +        set_error( async->iosb->status );
> +
> +        /* 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.
> +         *
> +         * NOTE: async_handoff() is being called with async->terminated = 1.
> +         */
> +        async->unknown_status = 0;
> +        async_handoff( async, NULL, 0 );

I kind of get the idea here, but my intuition tells me that this is a 
bad idea. I have to think too hard about what's involved in async_handoff().

Do we even get anything out of this that's not covered by 
async_set_initial_status(), plus async_set_result() and the wait_handle 
management below?
> +
> +        /* If the I/O has completed successfully, the client would have already
> +         * set the IOSB. Therefore, we can skip waiting on wait_handle and do
> +         * async_set_result() directly.
> +         */
> +        async_set_result( &async->obj, async->iosb->status, async->iosb->result );
> +
> +        /* Close wait handle here to avoid extra server round trip if the I/O
> +         * has completed successfully.
> +         *
> +         * - If STATUS_PENDING, async_handoff() decides whether to close
> +         *   the wait handle for us.  The wait handle is kept open if
> +         *   async->blocking is set (i.e. we're going to block on it).
> +         *
> +         * - If failed (NT_ERROR), async_handoff() always closes
> +         *   the wait_handle, which means async->wait_handle == 0.
> +         */
> +        if (async->wait_handle && async->iosb->status != STATUS_PENDING)

Isn't the first part of this condition redundant? If the client gave us 
a handle in the first place then async->wait_handle must be nonzero.
> +        {
> +            close_handle( async->thread->process, async->wait_handle );
> +            async->wait_handle = 0;
> +        }
> +    } /* Otherwise, async have been abruptly transitioned (cancelled?). */
> +
> +    /* async_set_result() may have overriden the error value.  Reset it. */
> +    set_error( async->iosb->status );

Does the client even need to care what this request returns?
> +    reply->handle = async->wait_handle;
> +
> +    release_object( &async->obj );
> +}
> diff --git a/server/protocol.def b/server/protocol.def
> index 02e73047f9b..d79eca074a0 100644
> --- a/server/protocol.def
> +++ b/server/protocol.def
> @@ -2163,6 +2163,16 @@ enum message_type
>   @END
>
> +/* Notify direct completion of async and close the wait handle if not blocking */
> +(a)REQ(notify_async_direct_result)

I might recommend calling this "set" instead of "notify". Consider e.g. 
"set_irp_result".
> +    obj_handle_t   handle;        /* wait handle */
> +    unsigned int   status;        /* completion status */
> +    apc_param_t    information;   /* IO_STATUS_BLOCK Information */
> +(a)REPLY
> +    obj_handle_t   handle;        /* wait handle, or NULL if closed */

Why do we need this?
> +(a)END
> +
> +
>   /* Perform a read on a file object */
>   @REQ(read)
>       async_data_t   async;         /* async I/O parameters */

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

[Jinoh Kang]

On 2/3/22 08:45, Zebediah Figura wrote:
> On 1/29/22 01:47, Jinoh Kang wrote:

...
> > diff --git a/server/async.c b/server/async.c
> > index e169bb23225..8852ff1c6d5 100644
> > --- a/server/async.c
> > +++ b/server/async.c
> > @@ -752,3 +752,65 @@ DECL_HANDLER(get_async_result)
> >       }
> >       set_error( iosb->status );
> >   }
> > +
> > +/* Notify direct completion of async and close the wait handle if not blocking */
> > +DECL_HANDLER(notify_async_direct_result)
> > +{
> > +    struct async *async = (struct async *)get_handle_obj( current->process, req->handle, 0, &async_ops );
> > +
> > +    if (!async) return;
> > +
> > +    /* we only return an async handle for asyncs created via create_request_async() */
> > +    assert( async->iosb );
> > +
> > +    if (async->unknown_status && !async->pending &&
> > +        async->terminated && async->alerted &&
> > +        async->iosb->status == STATUS_PENDING)
>
> So, these are all things that happen to be true assuming the client is behaving correctly,

We still need to assert "async->terminated && async->alerted" so that "async_set_result()" works correctly on STATUS_PENDING case.
The last one seems actually unnecessary though (looks like a leftover from v3 patch, my apologies).
> but the way this is written doesn't exactly make that clear (in particular, with a large if block, I can't see what the "else" case looks like immediately). I would recommend inverting the condition,

Sounds good.
> returning early, and possibly returning some error condition (STATUS_INVALID_PARAMETER?) to make it clear that the client is misbehaving.

In that case, I'd like to hear some suggestions on how the client would handle (or not handle) the "STATUS_INVALID_PARAMETER" error.
> v3 -> v4:
>     - make robust against abrupt transition/cancellation:
>       don't return STATUS_ACCESS_DENIED on precondition failure
>       (client won't know what to do with STATUS_ACCESS_DENIED anyway)

> Note also that "async->pending" is kind of irrelevant here, it semantically is only meaningful when considering things like whether the request should queue completion. I.e. it can kind of be considered undefined if async->unknown_status == 1.

"async->unknown_status" should imply "!async->pending" anyway, so I see what you're saying here.
> I'm not sure async->iosb->status is meaningful either, is it? We check for "iosb->status == STATUS_PENDING" in a few places, but I'm not really sure that's a good idea; we should probably be treating it more like "the value that gets returned in the userspace IOSB".

I concur.
> In any case I don't think it has semantic value here.
> > +    {
> > +        /* async->terminated is 1, so async_terminate() won't do anything.
> > +         * We need to set the status and result for ourselves.
> > +         */
> > +        async->iosb->status = req->status;
> > +        async->iosb->result = req->information;
>
> Except that neither of these variables actually matter past async_set_result(), right? We shouldn't need to bother.

async_handoff() is one of those few places where 'we check for "iosb->status == STATUS_PENDING"'.
Since the "async_terminate()" call inside "async_handoff()" doesn't work here (async is in terminated state), the async->iosb->status value set here will be used to determine the eventual pending status of async. If STATUS_PENDING, some necessary flags (async->direct_result and async->pending) will also be set/cleared.
Note that the set_error() below is still required so that initial_status is properly set and synchronous failure case properly done, etc.
> > +
> > +        /* Set status for async_handoff(). */
> > +        set_error( async->iosb->status );
> > +
> > +        /* 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.
> > +         *
> > +         * NOTE: async_handoff() is being called with async->terminated = 1.
> > +         */
> > +        async->unknown_status = 0;
> > +        async_handoff( async, NULL, 0 );
>
> I kind of get the idea here, but my intuition tells me that this is a bad idea. I have to think too hard about what's involved in async_handoff().

You're right--I just had to explain that above and it already sounds too complicated.  Perhaps only open code the relevant parts?
> Do we even get anything out of this that's not covered by async_set_initial_status(), plus async_set_result() and the wait_handle management below?

Yes.  Especially this part:
> if (async->iosb->status != STATUS_PENDING)
>    {
>        if (result) *result = async->iosb->result;
>        async->signaled = 1;
>    }
>    else
>    {
>        async->direct_result = 0;
>        async->pending = 1;
>        if (!async->blocking)
>        {
>            close_handle( async->thread->process, async->wait_handle);
>            async->wait_handle = 0;
>        }
>    }

The flags need to be properly set (especially in the else branch) so that the subsequent wait_async() would properly function.
Also note the blocking part, synchronous vs. asynchronous failure, etc.
> > +
> > +        /* If the I/O has completed successfully, the client would have already
> > +         * set the IOSB. Therefore, we can skip waiting on wait_handle and do
> > +         * async_set_result() directly.
> > +         */
> > +        async_set_result( &async->obj, async->iosb->status, async->iosb->result );
> > +
> > +        /* Close wait handle here to avoid extra server round trip if the I/O
> > +         * has completed successfully.
> > +         *
> > +         * - If STATUS_PENDING, async_handoff() decides whether to close
> > +         *   the wait handle for us.  The wait handle is kept open if
> > +         *   async->blocking is set (i.e. we're going to block on it).
> > +         *
> > +         * - If failed (NT_ERROR), async_handoff() always closes
> > +         *   the wait_handle, which means async->wait_handle == 0.
> > +         */
> > +        if (async->wait_handle && async->iosb->status != STATUS_PENDING)
>
> Isn't the first part of this condition redundant?

It isn't.
> If the client gave us a handle in the first place then async->wait_handle must be nonzero.

It can be set to zero in the middle by async_handoff()  (see above).
> > +        {
> > +            close_handle( async->thread->process, async->wait_handle );
> > +            async->wait_handle = 0;
> > +        }
> > +    } /* Otherwise, async have been abruptly transitioned (cancelled?). */
> > +
> > +    /* async_set_result() may have overriden the error value.  Reset it. */
> > +    set_error( async->iosb->status );
>
> Does the client even need to care what this request returns?

I don't think it normally should, but I don't exactly feel easy about silencing error from the server, either.
> > +    reply->handle = async->wait_handle;
> > +
> > +    release_object( &async->obj );
> > +}
> > diff --git a/server/protocol.def b/server/protocol.def
> > index 02e73047f9b..d79eca074a0 100644
> > --- a/server/protocol.def
> > +++ b/server/protocol.def
> > @@ -2163,6 +2163,16 @@ enum message_type
> >   @END
> >     +/* Notify direct completion of async and close the wait handle if not blocking */
> > +(a)REQ(notify_async_direct_result)
>
> I might recommend calling this "set" instead of "notify". Consider e.g. "set_irp_result".
> > +    obj_handle_t   handle;        /* wait handle */
> > +    unsigned int   status;        /* completion status */
> > +    apc_param_t    information;   /* IO_STATUS_BLOCK Information */
> > +(a)REPLY
> > +    obj_handle_t   handle;        /* wait handle, or NULL if closed */
>
> Why do we need this?

We could have just returned a boolean to indicate whether the wait handle has been closed by us.
(Note: the wait handle is closed when we're transitioning to STATUS_PENDING, _and_ async->blocking == 0.)
However, I found that just returning either the original handle or NULL to indicate that simplifies code a lot.
> > +(a)END
> > +
> > +
> >   /* Perform a read on a file object */
> >   @REQ(read)
> >       async_data_t   async;         /* async I/O parameters */

-- 
Sincerely,
Jinoh Kang

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

[Zebediah Figura]

On 2/3/22 10:23, Jinoh Kang wrote:
> In that case, I'd like to hear some suggestions on how the client would handle (or not handle) the "STATUS_INVALID_PARAMETER" error.

In a sense the client doesn't need to. If the client is behaving 
correctly it shouldn't happen. In that case I think it's fine to 
assert() [or ERR(), or just ignore the status]—we just don't want the 
*server* to assert() if the *client* misbehaves. [One day in the far off 
future the server may be a higher-privileged process.]
> > > +
> > > +        /* Set status for async_handoff(). */
> > > +        set_error( async->iosb->status );
> > > +
> > > +        /* 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.
> > > +         *
> > > +         * NOTE: async_handoff() is being called with async->terminated = 1.
> > > +         */
> > > +        async->unknown_status = 0;
> > > +        async_handoff( async, NULL, 0 );
> >
> > I kind of get the idea here, but my intuition tells me that this is a bad idea. I have to think too hard about what's involved in async_handoff().
>
> You're right--I just had to explain that above and it already sounds too complicated.  Perhaps only open code the relevant parts?
> > Do we even get anything out of this that's not covered by async_set_initial_status(), plus async_set_result() and the wait_handle management below?
>
> Yes.  Especially this part:
> > if (async->iosb->status != STATUS_PENDING)
> >     {
> >         if (result) *result = async->iosb->result;
> >         async->signaled = 1;
> >     }
> >     else
> >     {
> >         async->direct_result = 0;
> >         async->pending = 1;
> >         if (!async->blocking)
> >         {
> >             close_handle( async->thread->process, async->wait_handle);
> >             async->wait_handle = 0;
> >         }
> >     }
>
> The flags need to be properly set (especially in the else branch) so that the subsequent wait_async() would properly function.
> Also note the blocking part, synchronous vs. asynchronous failure, etc.

Of course I haven't tried it, but I think it probably wouldn't be too 
bad to open-code the relevant parts.
> > > +
> > > +        /* If the I/O has completed successfully, the client would have already
> > > +         * set the IOSB. Therefore, we can skip waiting on wait_handle and do
> > > +         * async_set_result() directly.
> > > +         */
> > > +        async_set_result( &async->obj, async->iosb->status, async->iosb->result );
> > > +
> > > +        /* Close wait handle here to avoid extra server round trip if the I/O
> > > +         * has completed successfully.
> > > +         *
> > > +         * - If STATUS_PENDING, async_handoff() decides whether to close
> > > +         *   the wait handle for us.  The wait handle is kept open if
> > > +         *   async->blocking is set (i.e. we're going to block on it).
> > > +         *
> > > +         * - If failed (NT_ERROR), async_handoff() always closes
> > > +         *   the wait_handle, which means async->wait_handle == 0.
> > > +         */
> > > +        if (async->wait_handle && async->iosb->status != STATUS_PENDING)
> >
> > Isn't the first part of this condition redundant?
>
> It isn't.
> > If the client gave us a handle in the first place then async->wait_handle must be nonzero.
>
> It can be set to zero in the middle by async_handoff()  (see above).

Ah right, async_handoff() closes the handle early in the case of 
synchronous failure. Don't know how I missed that yesterday.

Open-coding sort of obviates this point, though. <shrug>
> > > +        {
> > > +            close_handle( async->thread->process, async->wait_handle );
> > > +            async->wait_handle = 0;
> > > +        }
> > > +    } /* Otherwise, async have been abruptly transitioned (cancelled?). */
> > > +
> > > +    /* async_set_result() may have overriden the error value.  Reset it. */
> > > +    set_error( async->iosb->status );
> >
> > Does the client even need to care what this request returns?
>
> I don't think it normally should, but I don't exactly feel easy about silencing error from the server, either.

I guess. If that's a concern, it seems potentially clearer to clear the 
error instead if it's not going to be used by the client.

I.e. if you return the same status as the client gives you, my intuitive 
reading is "this function returns the status that the client should 
return, which is normally the same status as the client gives us but 
might be different in some cases." But then I can't tell what those 
cases are.
> > > +    reply->handle = async->wait_handle;
> > > +
> > > +    release_object( &async->obj );
> > > +}
> > > diff --git a/server/protocol.def b/server/protocol.def
> > > index 02e73047f9b..d79eca074a0 100644
> > > --- a/server/protocol.def
> > > +++ b/server/protocol.def
> > > @@ -2163,6 +2163,16 @@ enum message_type
> > >    @END
> > >      +/* Notify direct completion of async and close the wait handle if not blocking */
> > > +(a)REQ(notify_async_direct_result)
> >
> > I might recommend calling this "set" instead of "notify". Consider e.g. "set_irp_result".
> > > +    obj_handle_t   handle;        /* wait handle */
> > > +    unsigned int   status;        /* completion status */
> > > +    apc_param_t    information;   /* IO_STATUS_BLOCK Information */
> > > +(a)REPLY
> > > +    obj_handle_t   handle;        /* wait handle, or NULL if closed */
> >
> > Why do we need this?
>
> We could have just returned a boolean to indicate whether the wait handle has been closed by us.
> (Note: the wait handle is closed when we're transitioning to STATUS_PENDING, _and_ async->blocking == 0.)
> However, I found that just returning either the original handle or NULL to indicate that simplifies code a lot.

Oh, right, blocking asyncs are a concern, and even for this one...

Returning the wait handle like that does feel weird, and we don't really 
have a precedent for doing anything like this, but I'll leave it alone I 
guess.

[RFC PATCH v4 3/5] server: Attempt to complete I/O request immediately in recv_socket.

[Jinoh Kang]

Make recv_socket alert the async immediately if poll() call detects that
there are incoming data in the socket, bypassing the wineserver's main
polling loop.

Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com>
---

Notes:
    v3 -> v4: don't update IOSB on synchronous failure

 dlls/ntdll/unix/socket.c       | 22 ++++++++++++++++++----
 dlls/ntdll/unix/sync.c         | 22 ++++++++++++++++++++++
 dlls/ntdll/unix/unix_private.h |  1 +
 server/protocol.def            |  1 +
 server/sock.c                  | 23 ++++++++++++++++++++++-
 5 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c
index 71dfcdd1114..1c1ae25ab18 100644
--- a/dlls/ntdll/unix/socket.c
+++ b/dlls/ntdll/unix/socket.c
@@ -686,6 +686,7 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi
     NTSTATUS status;
     unsigned int i;
     ULONG options;
+    BOOL may_restart, alerted;
 
     if (unix_flags & MSG_OOB)
     {
@@ -756,16 +757,29 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi
         status = wine_server_call( req );
         wait_handle = wine_server_ptr_handle( reply->wait );
         options     = reply->options;
-        if ((!NT_ERROR(status) || wait_handle) && status != STATUS_PENDING)
+        may_restart = reply->may_restart;
+    }
+    SERVER_END_REQ;
+
+    alerted = status == STATUS_ALERTED;
+    if (alerted)
+    {
+        status = try_recv( fd, async, &information );
+        if (status == STATUS_DEVICE_NOT_READY && may_restart)
+            status = STATUS_PENDING;
+    }
+
+    if (status != STATUS_PENDING)
+    {
+        if (!NT_ERROR(status) || (wait_handle && !alerted))
         {
             io->Status = status;
             io->Information = information;
         }
+        release_fileio( &async->io );
     }
-    SERVER_END_REQ;
-
-    if (status != STATUS_PENDING) release_fileio( &async->io );
 
+    if (alerted) status = notify_async( &wait_handle, status, information );
     if (wait_handle) status = wait_async( wait_handle, options & FILE_SYNCHRONOUS_IO_ALERT );
     return status;
 }
diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c
index 442243d8bcf..4cb30479322 100644
--- a/dlls/ntdll/unix/sync.c
+++ b/dlls/ntdll/unix/sync.c
@@ -2510,3 +2510,25 @@ NTSTATUS WINAPI NtWaitForAlertByThreadId( const void *address, const LARGE_INTEG
 }
 
 #endif
+
+/* Notify direct completion of async and close the wait handle if it is no longer needed.
+ * This function is a no-op (returns status as-is) if the supplied handle is NULL.
+ */
+NTSTATUS notify_async( HANDLE *optional_handle, NTSTATUS status, ULONG_PTR information )
+{
+    NTSTATUS ret;
+
+    if (!*optional_handle) return status;
+
+    SERVER_START_REQ( notify_async_direct_result )
+    {
+        req->handle      = wine_server_obj_handle( *optional_handle );
+        req->status      = status;
+        req->information = information;
+        ret = wine_server_call( req );
+        *optional_handle = wine_server_ptr_handle( reply->handle );
+    }
+    SERVER_END_REQ;
+
+    return ret;
+}
diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h
index a79edabc37c..e8e158b5591 100644
--- a/dlls/ntdll/unix/unix_private.h
+++ b/dlls/ntdll/unix/unix_private.h
@@ -274,6 +274,7 @@ extern NTSTATUS get_device_info( int fd, struct _FILE_FS_DEVICE_INFORMATION *inf
 extern void init_files(void) DECLSPEC_HIDDEN;
 extern void init_cpu_info(void) DECLSPEC_HIDDEN;
 extern void add_completion( HANDLE handle, ULONG_PTR value, NTSTATUS status, ULONG info, BOOL async ) DECLSPEC_HIDDEN;
+extern NTSTATUS notify_async( HANDLE *optional_handle, NTSTATUS status, ULONG_PTR information );
 
 extern void dbg_init(void) DECLSPEC_HIDDEN;
 
diff --git a/server/protocol.def b/server/protocol.def
index d79eca074a0..137e6c5a220 100644
--- a/server/protocol.def
+++ b/server/protocol.def
@@ -1456,6 +1456,7 @@ enum server_fd_type
 @REPLY
     obj_handle_t wait;          /* handle to wait on for blocking recv */
     unsigned int options;       /* device open options */
+    int          may_restart;   /* May restart async? */
 @END
 
 
diff --git a/server/sock.c b/server/sock.c
index 40fb0cac535..14d95451420 100644
--- a/server/sock.c
+++ b/server/sock.c
@@ -3397,6 +3397,7 @@ DECL_HANDLER(recv_socket)
 {
     struct sock *sock = (struct sock *)get_handle_obj( current->process, req->async.handle, 0, &sock_ops );
     unsigned int status = req->status;
+    int pending = 0;
     timeout_t timeout = 0;
     struct async *async;
     struct fd *fd;
@@ -3422,6 +3423,25 @@ DECL_HANDLER(recv_socket)
     if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && sock->rd_shutdown)
         status = STATUS_PIPE_DISCONNECTED;
 
+    /* NOTE: If read_q is not empty, we cannot really tell if the already queued asyncs
+     * NOTE: will not consume all available data; if there's no data available,
+     * NOTE: the current request won't be immediately satiable. */
+    if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && !async_queued( &sock->read_q ))
+    {
+        struct pollfd pollfd;
+        pollfd.fd = get_unix_fd( sock->fd );
+        pollfd.events = req->oob ? POLLPRI : POLLIN;
+        pollfd.revents = 0;
+        if (poll(&pollfd, 1, 0) >= 0 && pollfd.revents)
+        {
+            /* Give the client opportunity to complete synchronously.
+             * If it turns out that the I/O request is not actually immediately satiable,
+             * the client may then choose to re-queue the async (with STATUS_PENDING). */
+            pending = status == STATUS_PENDING;
+            status = STATUS_ALERTED;
+        }
+    }
+
     sock->pending_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ);
     sock->reported_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ);
 
@@ -3438,7 +3458,7 @@ DECL_HANDLER(recv_socket)
         if (timeout)
             async_set_timeout( async, timeout, STATUS_IO_TIMEOUT );
 
-        if (status == STATUS_PENDING)
+        if (status == STATUS_PENDING || status == STATUS_ALERTED)
             queue_async( &sock->read_q, async );
 
         /* always reselect; we changed reported_events above */
@@ -3446,6 +3466,7 @@ DECL_HANDLER(recv_socket)
 
         reply->wait = async_handoff( async, NULL, 0 );
         reply->options = get_fd_options( fd );
+        reply->may_restart = pending && status == STATUS_ALERTED;
         release_object( async );
     }
     release_object( sock );
-- 
2.34.1

[RFC PATCH v4 3/5] server: Attempt to complete I/O request immediately in recv_socket.

[Zebediah Figura]

On 1/29/22 01:47, Jinoh Kang wrote:
> diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c
> index 442243d8bcf..4cb30479322 100644
> --- a/dlls/ntdll/unix/sync.c
> +++ b/dlls/ntdll/unix/sync.c
> @@ -2510,3 +2510,25 @@ NTSTATUS WINAPI NtWaitForAlertByThreadId( const void *address, const LARGE_INTEG
>   }
>
> #endif
> +
> +/* Notify direct completion of async and close the wait handle if it is no longer needed.
> + * This function is a no-op (returns status as-is) if the supplied handle is NULL.
> + */
> +NTSTATUS notify_async( HANDLE *optional_handle, NTSTATUS status, ULONG_PTR information )

This function seems rather ambiguously named. Why not use the same name 
as for the server request?
> +{
> +    NTSTATUS ret;
> +
> +    if (!*optional_handle) return status;
> +
> +    SERVER_START_REQ( notify_async_direct_result )
> +    {
> +        req->handle      = wine_server_obj_handle( *optional_handle );
> +        req->status      = status;
> +        req->information = information;
> +        ret = wine_server_call( req );
> +        *optional_handle = wine_server_ptr_handle( reply->handle );
> +    }
> +    SERVER_END_REQ;
> +
> +    return ret;
> +}

...
> diff --git a/server/protocol.def b/server/protocol.def
> index d79eca074a0..137e6c5a220 100644
> --- a/server/protocol.def
> +++ b/server/protocol.def
> @@ -1456,6 +1456,7 @@ enum server_fd_type
>   @REPLY
>       obj_handle_t wait;          /* handle to wait on for blocking recv */
>       unsigned int options;       /* device open options */
> +    int          may_restart;   /* May restart async? */
>   @END

"may_restart" takes me a minute to realize what it actually means. Could 
we just return the (non)blocking flag directly instead?

[RFC PATCH v4 3/5] server: Attempt to complete I/O request immediately in recv_socket.

[Jinoh Kang]

On 2/3/22 08:45, Zebediah Figura wrote:
> On 1/29/22 01:47, Jinoh Kang wrote:
> > diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c
> > index 442243d8bcf..4cb30479322 100644
> > --- a/dlls/ntdll/unix/sync.c
> > +++ b/dlls/ntdll/unix/sync.c
> > @@ -2510,3 +2510,25 @@ NTSTATUS WINAPI NtWaitForAlertByThreadId( const void *address, const LARGE_INTEG
> >   }
> >     #endif
> > +
> > +/* Notify direct completion of async and close the wait handle if it is no longer needed.
> > + * This function is a no-op (returns status as-is) if the supplied handle is NULL.
> > + */
> > +NTSTATUS notify_async( HANDLE *optional_handle, NTSTATUS status, ULONG_PTR information )
>
> This function seems rather ambiguously named. Why not use the same name as for the server request?

Ack.
> > +{
> > +    NTSTATUS ret;
> > +
> > +    if (!*optional_handle) return status;
> > +
> > +    SERVER_START_REQ( notify_async_direct_result )
> > +    {
> > +        req->handle      = wine_server_obj_handle( *optional_handle );
> > +        req->status      = status;
> > +        req->information = information;
> > +        ret = wine_server_call( req );
> > +        *optional_handle = wine_server_ptr_handle( reply->handle );
> > +    }
> > +    SERVER_END_REQ;
> > +
> > +    return ret;
> > +}
>
> ...
> > diff --git a/server/protocol.def b/server/protocol.def
> > index d79eca074a0..137e6c5a220 100644
> > --- a/server/protocol.def
> > +++ b/server/protocol.def
> > @@ -1456,6 +1456,7 @@ enum server_fd_type
> >   @REPLY
> >       obj_handle_t wait;          /* handle to wait on for blocking recv */
> >       unsigned int options;       /* device open options */
> > +    int          may_restart;   /* May restart async? */
> >   @END
> >    
>
> "may_restart" takes me a minute to realize what it actually means. Could we just return the (non)blocking flag directly instead?

So we do "force_async || !nonblocking" at the client side, I guess?

-- 
Sincerely,
Jinoh Kang

[RFC PATCH v4 3/5] server: Attempt to complete I/O request immediately in recv_socket.

[Zebediah Figura]

On 2/3/22 10:26, Jinoh Kang wrote:
> > > diff --git a/server/protocol.def b/server/protocol.def
> > > index d79eca074a0..137e6c5a220 100644
> > > --- a/server/protocol.def
> > > +++ b/server/protocol.def
> > > @@ -1456,6 +1456,7 @@ enum server_fd_type
> > >    @REPLY
> > >        obj_handle_t wait;          /* handle to wait on for blocking recv */
> > >        unsigned int options;       /* device open options */
> > > +    int          may_restart;   /* May restart async? */
> > >    @END
> >
> > "may_restart" takes me a minute to realize what it actually means. Could we just return the (non)blocking flag directly instead?
>
> So we do "force_async || !nonblocking" at the client side, I guess?

Yeah.

I guess another alternative is just to call this field "blocking". That 
might apply to the server-side "pending" local variable also; maybe that 
would address one of my comments on 5/5.

[RFC PATCH v4 4/5] ntdll: Don't call try_recv before server call in sock_recv.

[Jinoh Kang]

Otherwise, try_recv() call from sock_recv() may race against try_recv()
call from async_recv_proc(), shuffling the packet order.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52401
Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com>
---

Notes:
    v3 -> v4: unchanged

 dlls/ntdll/unix/socket.c | 12 ++----------
 dlls/ws2_32/tests/sock.c |  8 ++++----
 2 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c
index 1c1ae25ab18..86f5d0d29a2 100644
--- a/dlls/ntdll/unix/socket.c
+++ b/dlls/ntdll/unix/socket.c
@@ -737,16 +737,8 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi
         }
     }
 
-    status = try_recv( fd, async, &information );
-
-    if (status != STATUS_SUCCESS && status != STATUS_BUFFER_OVERFLOW && status != STATUS_DEVICE_NOT_READY)
-    {
-        release_fileio( &async->io );
-        return status;
-    }
-
-    if (status == STATUS_DEVICE_NOT_READY && force_async)
-        status = STATUS_PENDING;
+    status = force_async ? STATUS_PENDING : STATUS_DEVICE_NOT_READY;
+    information = 0;
 
     SERVER_START_REQ( recv_socket )
     {
diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c
index 054e597b719..4199676f460 100644
--- a/dlls/ws2_32/tests/sock.c
+++ b/dlls/ws2_32/tests/sock.c
@@ -7750,8 +7750,8 @@ static void test_shutdown(void)
 
     WSASetLastError(0xdeadbeef);
     ret = recv(server, buffer, sizeof(buffer), 0);
-    todo_wine ok(ret == -1, "got %d\n", ret);
-    todo_wine ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError());
+    ok(ret == -1, "got %d\n", ret);
+    ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError());
 
     ret = send(server, "test", 5, 0);
     ok(ret == 5, "got %d\n", ret);
@@ -7845,8 +7845,8 @@ static void test_shutdown(void)
 
     WSASetLastError(0xdeadbeef);
     ret = recv(server, buffer, sizeof(buffer), 0);
-    todo_wine ok(ret == -1, "got %d\n", ret);
-    todo_wine ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError());
+    ok(ret == -1, "got %d\n", ret);
+    ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError());
 
     WSASetLastError(0xdeadbeef);
     ret = send(server, "test", 5, 0);
-- 
2.34.1

[RFC PATCH v4 5/5] server: Replace redundant recv_socket status fields with force_async boolean field.

[Jinoh Kang]

The 'status' field of recv_socket_request is always either
STATUS_PENDING or STATUS_DEVICE_NOT_READY, and the 'total' field is
always zero.

Replace the 'status' field with 'force_async' boolean field, and get rid
of the 'total' field entirely.

Also, clean up the recv_socket handler code a bit.

Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com>
---

Notes:
    v3 -> v4: unchanged

 dlls/ntdll/unix/socket.c |  7 ++-----
 server/protocol.def      |  3 +--
 server/sock.c            | 23 +++++++++--------------
 3 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c
index 86f5d0d29a2..4cebb4e7829 100644
--- a/dlls/ntdll/unix/socket.c
+++ b/dlls/ntdll/unix/socket.c
@@ -737,13 +737,9 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi
         }
     }
 
-    status = force_async ? STATUS_PENDING : STATUS_DEVICE_NOT_READY;
-    information = 0;
-
     SERVER_START_REQ( recv_socket )
     {
-        req->status = status;
-        req->total  = information;
+        req->force_async = force_async;
         req->async  = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) );
         req->oob    = !!(unix_flags & MSG_OOB);
         status = wine_server_call( req );
@@ -753,6 +749,7 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi
     }
     SERVER_END_REQ;
 
+    information = 0;
     alerted = status == STATUS_ALERTED;
     if (alerted)
     {
diff --git a/server/protocol.def b/server/protocol.def
index 137e6c5a220..2b870ae22e9 100644
--- a/server/protocol.def
+++ b/server/protocol.def
@@ -1451,8 +1451,7 @@ enum server_fd_type
 @REQ(recv_socket)
     int          oob;           /* are we receiving OOB data? */
     async_data_t async;         /* async I/O parameters */
-    unsigned int status;        /* status of initial call */
-    unsigned int total;         /* number of bytes already read */
+    int          force_async;   /* Force asynchronous mode? */
 @REPLY
     obj_handle_t wait;          /* handle to wait on for blocking recv */
     unsigned int options;       /* device open options */
diff --git a/server/sock.c b/server/sock.c
index 14d95451420..fbf76870a51 100644
--- a/server/sock.c
+++ b/server/sock.c
@@ -3396,8 +3396,8 @@ struct object *create_socket_device( struct object *root, const struct unicode_s
 DECL_HANDLER(recv_socket)
 {
     struct sock *sock = (struct sock *)get_handle_obj( current->process, req->async.handle, 0, &sock_ops );
-    unsigned int status = req->status;
-    int pending = 0;
+    unsigned int status = STATUS_PENDING;
+    int pending = req->force_async;
     timeout_t timeout = 0;
     struct async *async;
     struct fd *fd;
@@ -3405,8 +3405,8 @@ DECL_HANDLER(recv_socket)
     if (!sock) return;
     fd = sock->fd;
 
-    /* recv() returned EWOULDBLOCK, i.e. no data available yet */
-    if (status == STATUS_DEVICE_NOT_READY && !sock->nonblocking)
+    /* Synchronous, *blocking* I/O requested? */
+    if (!req->force_async && !sock->nonblocking)
     {
         /* Set a timeout on the async if necessary.
          *
@@ -3417,16 +3417,16 @@ DECL_HANDLER(recv_socket)
         if (is_fd_overlapped( fd ))
             timeout = (timeout_t)sock->rcvtimeo * -10000;
 
-        status = STATUS_PENDING;
+        pending = 1;
     }
 
-    if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && sock->rd_shutdown)
+    if (status == STATUS_PENDING && sock->rd_shutdown)
         status = STATUS_PIPE_DISCONNECTED;
 
     /* NOTE: If read_q is not empty, we cannot really tell if the already queued asyncs
      * NOTE: will not consume all available data; if there's no data available,
      * NOTE: the current request won't be immediately satiable. */
-    if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && !async_queued( &sock->read_q ))
+    if (status == STATUS_PENDING && !async_queued( &sock->read_q ))
     {
         struct pollfd pollfd;
         pollfd.fd = get_unix_fd( sock->fd );
@@ -3437,22 +3437,17 @@ DECL_HANDLER(recv_socket)
             /* Give the client opportunity to complete synchronously.
              * If it turns out that the I/O request is not actually immediately satiable,
              * the client may then choose to re-queue the async (with STATUS_PENDING). */
-            pending = status == STATUS_PENDING;
             status = STATUS_ALERTED;
         }
     }
 
+    if (!pending && status == STATUS_PENDING) status = STATUS_DEVICE_NOT_READY;
+
     sock->pending_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ);
     sock->reported_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ);
 
     if ((async = create_request_async( fd, get_fd_comp_flags( fd ), &req->async )))
     {
-        if (status == STATUS_SUCCESS)
-        {
-            struct iosb *iosb = async_get_iosb( async );
-            iosb->result = req->total;
-            release_object( iosb );
-        }
         set_error( status );
 
         if (timeout)
-- 
2.34.1

[RFC PATCH v4 5/5] server: Replace redundant recv_socket status fields with force_async boolean field.

[Zebediah Figura]

On 1/29/22 01:47, Jinoh Kang wrote:
> diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c
> index 86f5d0d29a2..4cebb4e7829 100644
> --- a/dlls/ntdll/unix/socket.c
> +++ b/dlls/ntdll/unix/socket.c
> @@ -737,13 +737,9 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi
>           }
>       }
>
> -    status = force_async ? STATUS_PENDING : STATUS_DEVICE_NOT_READY;
> -    information = 0;
> -
>       SERVER_START_REQ( recv_socket )
>       {
> -        req->status = status;
> -        req->total  = information;
> +        req->force_async = force_async;
>           req->async  = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) );
>           req->oob    = !!(unix_flags & MSG_OOB);
>           status = wine_server_call( req );
> @@ -753,6 +749,7 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi
>       }
>       SERVER_END_REQ;
>
> +    information = 0;
>       alerted = status == STATUS_ALERTED;
>       if (alerted)
>       {

Do we still need this?
> diff --git a/server/protocol.def b/server/protocol.def
> index 137e6c5a220..2b870ae22e9 100644
> --- a/server/protocol.def
> +++ b/server/protocol.def
> @@ -1451,8 +1451,7 @@ enum server_fd_type
>   @REQ(recv_socket)
>       int          oob;           /* are we receiving OOB data? */
>       async_data_t async;         /* async I/O parameters */
> -    unsigned int status;        /* status of initial call */
> -    unsigned int total;         /* number of bytes already read */
> +    int          force_async;   /* Force asynchronous mode? */
>   @REPLY
>       obj_handle_t wait;          /* handle to wait on for blocking recv */
>       unsigned int options;       /* device open options */
> diff --git a/server/sock.c b/server/sock.c
> index 14d95451420..fbf76870a51 100644
> --- a/server/sock.c
> +++ b/server/sock.c
> @@ -3396,8 +3396,8 @@ struct object *create_socket_device( struct object *root, const struct unicode_s
>   DECL_HANDLER(recv_socket)
>   {
>       struct sock *sock = (struct sock *)get_handle_obj( current->process, req->async.handle, 0, &sock_ops );
> -    unsigned int status = req->status;
> -    int pending = 0;
> +    unsigned int status = STATUS_PENDING;
> +    int pending = req->force_async;
>       timeout_t timeout = 0;
>       struct async *async;
>       struct fd *fd;
> @@ -3405,8 +3405,8 @@ DECL_HANDLER(recv_socket)
>       if (!sock) return;
>       fd = sock->fd;
>
> -    /* recv() returned EWOULDBLOCK, i.e. no data available yet */
> -    if (status == STATUS_DEVICE_NOT_READY && !sock->nonblocking)
> +    /* Synchronous, *blocking* I/O requested? */
> +    if (!req->force_async && !sock->nonblocking)
>       {
>           /* Set a timeout on the async if necessary.
>            *
> @@ -3417,16 +3417,16 @@ DECL_HANDLER(recv_socket)
>           if (is_fd_overlapped( fd ))
>               timeout = (timeout_t)sock->rcvtimeo * -10000;
>
> -        status = STATUS_PENDING;
> +        pending = 1;
>       }
>
> -    if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && sock->rd_shutdown)
> +    if (status == STATUS_PENDING && sock->rd_shutdown)

I may be misreading difs, but isn't this check for STATUS_PENDING 
redundant now?
> status = STATUS_PIPE_DISCONNECTED;
>
> /* NOTE: If read_q is not empty, we cannot really tell if the already queued asyncs
>        * NOTE: will not consume all available data; if there's no data available,
>        * NOTE: the current request won't be immediately satiable. */
> -    if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && !async_queued( &sock->read_q ))
> +    if (status == STATUS_PENDING && !async_queued( &sock->read_q ))
>       {
>           struct pollfd pollfd;
>           pollfd.fd = get_unix_fd( sock->fd );

And if so this could probably just become an "else" block.
> @@ -3437,22 +3437,17 @@ DECL_HANDLER(recv_socket)
>               /* Give the client opportunity to complete synchronously.
>                * If it turns out that the I/O request is not actually immediately satiable,
>                * the client may then choose to re-queue the async (with STATUS_PENDING). */
> -            pending = status == STATUS_PENDING;
>               status = STATUS_ALERTED;
>           }
>       }
>
> +    if (!pending && status == STATUS_PENDING) status = STATUS_DEVICE_NOT_READY;
> +

I feel like it's hard to immediately tell what this is doing (e.g. what 
does "pending" mean?)

Can this be folded into the conditional that deals with timeouts somehow?
> sock->pending_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ);
>       sock->reported_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ);
>
> if ((async = create_request_async( fd, get_fd_comp_flags( fd ), &req->async )))
>       {
> -        if (status == STATUS_SUCCESS)
> -        {
> -            struct iosb *iosb = async_get_iosb( async );
> -            iosb->result = req->total;
> -            release_object( iosb );
> -        }
>           set_error( status );
>
> if (timeout)

[RFC PATCH v4 5/5] server: Replace redundant recv_socket status fields with force_async boolean field.

[Jinoh Kang]

On 2/3/22 08:45, Zebediah Figura wrote:
> On 1/29/22 01:47, Jinoh Kang wrote:
> > diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c
> > index 86f5d0d29a2..4cebb4e7829 100644
> > --- a/dlls/ntdll/unix/socket.c
> > +++ b/dlls/ntdll/unix/socket.c
> > @@ -737,13 +737,9 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi
> >           }
> >       }
> >   -    status = force_async ? STATUS_PENDING : STATUS_DEVICE_NOT_READY;
> > -    information = 0;
> > -
> >       SERVER_START_REQ( recv_socket )
> >       {
> > -        req->status = status;
> > -        req->total  = information;
> > +        req->force_async = force_async;
> >           req->async  = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) );
> >           req->oob    = !!(unix_flags & MSG_OOB);
> >           status = wine_server_call( req );
> > @@ -753,6 +749,7 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi
> >       }
> >       SERVER_END_REQ;
> >   +    information = 0;
> >       alerted = status == STATUS_ALERTED;
> >       if (alerted)
> >       {
>
> Do we still need this?

I suppose I can drop it and leave it uninitialised, as long as GCC is happy about it...
> > diff --git a/server/protocol.def b/server/protocol.def
> > index 137e6c5a220..2b870ae22e9 100644
> > --- a/server/protocol.def
> > +++ b/server/protocol.def
> > @@ -1451,8 +1451,7 @@ enum server_fd_type
> >   @REQ(recv_socket)
> >       int          oob;           /* are we receiving OOB data? */
> >       async_data_t async;         /* async I/O parameters */
> > -    unsigned int status;        /* status of initial call */
> > -    unsigned int total;         /* number of bytes already read */
> > +    int          force_async;   /* Force asynchronous mode? */
> >   @REPLY
> >       obj_handle_t wait;          /* handle to wait on for blocking recv */
> >       unsigned int options;       /* device open options */
> > diff --git a/server/sock.c b/server/sock.c
> > index 14d95451420..fbf76870a51 100644
> > --- a/server/sock.c
> > +++ b/server/sock.c
> > @@ -3396,8 +3396,8 @@ struct object *create_socket_device( struct object *root, const struct unicode_s
> >   DECL_HANDLER(recv_socket)
> >   {
> >       struct sock *sock = (struct sock *)get_handle_obj( current->process, req->async.handle, 0, &sock_ops );
> > -    unsigned int status = req->status;
> > -    int pending = 0;
> > +    unsigned int status = STATUS_PENDING;
> > +    int pending = req->force_async;
> >       timeout_t timeout = 0;
> >       struct async *async;
> >       struct fd *fd;
> > @@ -3405,8 +3405,8 @@ DECL_HANDLER(recv_socket)
> >       if (!sock) return;
> >       fd = sock->fd;
> >   -    /* recv() returned EWOULDBLOCK, i.e. no data available yet */
> > -    if (status == STATUS_DEVICE_NOT_READY && !sock->nonblocking)
> > +    /* Synchronous, *blocking* I/O requested? */
> > +    if (!req->force_async && !sock->nonblocking)
> >       {
> >           /* Set a timeout on the async if necessary.
> >            *
> > @@ -3417,16 +3417,16 @@ DECL_HANDLER(recv_socket)
> >           if (is_fd_overlapped( fd ))
> >               timeout = (timeout_t)sock->rcvtimeo * -10000;
> >   -        status = STATUS_PENDING;
> > +        pending = 1;
> >       }
> >   -    if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && sock->rd_shutdown)
> > +    if (status == STATUS_PENDING && sock->rd_shutdown)
>
> I may be misreading difs, but isn't this check for STATUS_PENDING redundant now?

It is.  I just left it so it's easy to reorder those if blocks, now that I think about it, it wouldn't be necessary...
> >           status = STATUS_PIPE_DISCONNECTED;
> >         /* NOTE: If read_q is not empty, we cannot really tell if the already queued asyncs
> >        * NOTE: will not consume all available data; if there's no data available,
> >        * NOTE: the current request won't be immediately satiable. */
> > -    if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && !async_queued( &sock->read_q ))
> > +    if (status == STATUS_PENDING && !async_queued( &sock->read_q ))
> >       {
> >           struct pollfd pollfd;
> >           pollfd.fd = get_unix_fd( sock->fd );
>
> And if so this could probably just become an "else" block.

Ack.
> > @@ -3437,22 +3437,17 @@ DECL_HANDLER(recv_socket)
> >               /* Give the client opportunity to complete synchronously.
> >                * If it turns out that the I/O request is not actually immediately satiable,
> >                * the client may then choose to re-queue the async (with STATUS_PENDING). */
> > -            pending = status == STATUS_PENDING;
> >               status = STATUS_ALERTED;
> >           }
> >       }
> >   +    if (!pending && status == STATUS_PENDING) status = STATUS_DEVICE_NOT_READY;
> > +
>
> I feel like it's hard to immediately tell what this is doing (e.g. what does "pending" mean?)
>
> Can this be folded into the conditional that deals with timeouts somehow?

I'll try.  Maybe we can move the rd_shutdown check to the top.
> >       sock->pending_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ);
> >       sock->reported_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ);
> >         if ((async = create_request_async( fd, get_fd_comp_flags( fd ), &req->async )))
> >       {
> > -        if (status == STATUS_SUCCESS)
> > -        {
> > -            struct iosb *iosb = async_get_iosb( async );
> > -            iosb->result = req->total;
> > -            release_object( iosb );
> > -        }
> >           set_error( status );
> >             if (timeout)

-- 
Sincerely,
Jinoh Kang