[RFC PATCH v2 0/5] Fix sock_recv reordering issue (#52401): unknown_status + wineserver call approach
<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 [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 | 106 ++++++++++++++++++++++++++++++--- server/protocol.def | 14 ++++- server/sock.c | 40 +++++++++---- 7 files changed, 185 insertions(+), 43 deletions(-) -- 2.34.1
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> --- server/async.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/server/async.c b/server/async.c index 7aef28355f0..6373e8c1ae4 100644 --- a/server/async.c +++ b/server/async.c @@ -338,6 +338,28 @@ 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_reselect( async ); + + return async->wait_handle; + } + async->initial_status = get_error(); if (!async->pending && NT_ERROR( get_error() )) -- 2.34.1
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 server/async.c | 84 ++++++++++++++++++++++++++++++++++++++++----- server/protocol.def | 10 ++++++ 2 files changed, 86 insertions(+), 8 deletions(-) diff --git a/server/async.c b/server/async.c index 6373e8c1ae4..714e82362c6 100644 --- a/server/async.c +++ b/server/async.c @@ -473,6 +473,17 @@ static void add_async_completion( struct async *async, apc_param_t cvalue, unsig if (async->completion) add_completion( async->completion, async->comp_key, cvalue, status, information ); } +static void async_dequeue( struct async *async ) +{ + if (!async->queue) return; + + list_remove( &async->queue_entry ); + async_reselect( async ); + async->fd = NULL; + async->queue = NULL; + release_object( async ); +} + /* store the result of the client-side async callback */ void async_set_result( struct object *obj, unsigned int status, apc_param_t total ) { @@ -531,14 +542,7 @@ void async_set_result( struct object *obj, unsigned int status, apc_param_t tota async->completion_callback( async->completion_callback_private ); async->completion_callback = NULL; - if (async->queue) - { - list_remove( &async->queue_entry ); - async_reselect( async ); - async->fd = NULL; - async->queue = NULL; - release_object( async ); - } + async_dequeue( async ); } } @@ -750,3 +754,67 @@ DECL_HANDLER(get_async_result) } set_error( iosb->status ); } + +/* Notify direct completion of async and close the wait handle */ +DECL_HANDLER(notify_async_direct_result) +{ + struct async *async = (struct async *)get_handle_obj( current->process, req->handle, 0, &async_ops ); + + if (!async) return; + + if (async->iosb && async->unknown_status && !async->pending && async->terminated) + { + /* Reactivate async. We call async_reselect() later. */ + async->terminated = 0; + + /* Set result for async_handoff(). */ + set_error( req->status ); + async->iosb->result = req->information; + + /* 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. + */ + async->unknown_status = 0; + async_handoff( async, NULL, 0 ); + + if (get_error() == STATUS_PENDING) + { + async_reselect( async ); + } + else if (NT_ERROR( get_error() )) + { + /* synchronous I/O failure: don't invoke callbacks, only dequeue it. */ + async_dequeue( async ); + } + else + { + /* I/O completed successfully. The client has already set the IOSB, + * so we can skip waiting on wait_handle and do async_set_result() + * directly. + * + * If !async->direct_result, an APC_ASYNC_IO has been fired. + * async_set_result() will be called when the APC returns. + */ + if (async->direct_result) + { + async_set_result( &async->obj, async->iosb->status, async->iosb->result ); + async->direct_result = 0; + } + + /* close wait handle here to avoid extra server round trip */ + if (async->wait_handle) + { + close_handle( async->thread->process, async->wait_handle ); + async->wait_handle = 0; + } + } + + /* The wait handle is preserved only when the status is STATUS_PENDING + * and async->blocking is set (i.e. we're going to block on it). */ + reply->handle = async->wait_handle; + } + else set_error( STATUS_ACCESS_DENIED ); + + release_object( &async->obj ); +} diff --git a/server/protocol.def b/server/protocol.def index 02e73047f9b..2c3b8dbc619 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 */ +(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
On 1/28/22 04:05, 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
server/async.c | 84 ++++++++++++++++++++++++++++++++++++++++----- server/protocol.def | 10 ++++++ 2 files changed, 86 insertions(+), 8 deletions(-)
diff --git a/server/async.c b/server/async.c index 6373e8c1ae4..714e82362c6 100644 --- a/server/async.c +++ b/server/async.c @@ -473,6 +473,17 @@ static void add_async_completion( struct async *async, apc_param_t cvalue, unsig if (async->completion) add_completion( async->completion, async->comp_key, cvalue, status, information ); }
+static void async_dequeue( struct async *async ) +{ + if (!async->queue) return; + + list_remove( &async->queue_entry ); + async_reselect( async ); + async->fd = NULL; + async->queue = NULL; + release_object( async ); +} + /* store the result of the client-side async callback */ void async_set_result( struct object *obj, unsigned int status, apc_param_t total ) { @@ -531,14 +542,7 @@ void async_set_result( struct object *obj, unsigned int status, apc_param_t tota async->completion_callback( async->completion_callback_private ); async->completion_callback = NULL;
- if (async->queue) - { - list_remove( &async->queue_entry ); - async_reselect( async ); - async->fd = NULL; - async->queue = NULL; - release_object( async ); - } + async_dequeue( async ); } }
@@ -750,3 +754,67 @@ DECL_HANDLER(get_async_result) } set_error( iosb->status ); } + +/* Notify direct completion of async and close the wait handle */ +DECL_HANDLER(notify_async_direct_result) +{ + struct async *async = (struct async *)get_handle_obj( current->process, req->handle, 0, &async_ops ); + + if (!async) return; + + if (async->iosb && async->unknown_status && !async->pending && async->terminated) + { + /* Reactivate async. We call async_reselect() later. */ + async->terminated = 0; + + /* Set result for async_handoff(). */ + set_error( req->status ); + async->iosb->result = req->information; + + /* 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. + */ + async->unknown_status = 0; + async_handoff( async, NULL, 0 ); + + if (get_error() == STATUS_PENDING) + { + async_reselect( async ); + } + else if (NT_ERROR( get_error() )) + { + /* synchronous I/O failure: don't invoke callbacks, only dequeue it. */ + async_dequeue( async );
Note: we can't use async_set_result() here. async_handoff() leaves async->terminated as 0, and async_set_result() has "assert( async->terminated )". Thus, reusing async_set_result() here requires either: 1. Setting async->pending to 0 so that async_handoff() will call async_terminate(). This is obviously incorrect since unwanted IOCP packets and APCs will fire on synchronous failure. 2. Not using async_handoff(). We have to copy a lot of code (particulary setting pending and direct_result flags) out of async_handoff() to do this.
+ } + else + { + /* I/O completed successfully. The client has already set the IOSB, + * so we can skip waiting on wait_handle and do async_set_result() + * directly. + * + * If !async->direct_result, an APC_ASYNC_IO has been fired. + * async_set_result() will be called when the APC returns. + */ + if (async->direct_result) + { + async_set_result( &async->obj, async->iosb->status, async->iosb->result ); + async->direct_result = 0; + } + + /* close wait handle here to avoid extra server round trip */ + if (async->wait_handle) + { + close_handle( async->thread->process, async->wait_handle ); + async->wait_handle = 0; + } + } + + /* The wait handle is preserved only when the status is STATUS_PENDING + * and async->blocking is set (i.e. we're going to block on it). */ + reply->handle = async->wait_handle; + } + else set_error( STATUS_ACCESS_DENIED ); + + release_object( &async->obj ); +} diff --git a/server/protocol.def b/server/protocol.def index 02e73047f9b..2c3b8dbc619 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 */ +(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 */
-- Sincerely, Jinoh Kang
On 1/28/22 04:13, Jinoh Kang wrote:
On 1/28/22 04:05, Jinoh Kang wrote:
+ +/* Notify direct completion of async and close the wait handle */ +DECL_HANDLER(notify_async_direct_result) +{ + struct async *async = (struct async *)get_handle_obj( current->process, req->handle, 0, &async_ops ); + + if (!async) return; + + if (async->iosb && async->unknown_status && !async->pending && async->terminated) + { + /* Reactivate async. We call async_reselect() later. */ + async->terminated = 0; + + /* Set result for async_handoff(). */ + set_error( req->status ); + async->iosb->result = req->information; + + /* 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. + */ + async->unknown_status = 0; + async_handoff( async, NULL, 0 ); + + if (get_error() == STATUS_PENDING) + { + async_reselect( async ); + } + else if (NT_ERROR( get_error() )) + { + /* synchronous I/O failure: don't invoke callbacks, only dequeue it. */ + async_dequeue( async );
Note: we can't use async_set_result() here. async_handoff() leaves async->terminated as 0, and async_set_result() has "assert( async->terminated )".
Thus, reusing async_set_result() here requires either:
1. Setting async->pending to 0 so that async_handoff() will call async_terminate(). This is obviously incorrect since unwanted IOCP packets and APCs will fire on synchronous failure.
2. Not using async_handoff(). We have to copy a lot of code (particulary setting pending and direct_result flags) out of async_handoff() to do this.
Maybe just merge both branches (error/success) and force async_terminate() before calling async_set_result()?
+ } + else + { + /* I/O completed successfully. The client has already set the IOSB, + * so we can skip waiting on wait_handle and do async_set_result() + * directly. + * + * If !async->direct_result, an APC_ASYNC_IO has been fired. + * async_set_result() will be called when the APC returns. + */ + if (async->direct_result) + { + async_set_result( &async->obj, async->iosb->status, async->iosb->result ); + async->direct_result = 0; + } + + /* close wait handle here to avoid extra server round trip */ + if (async->wait_handle) + { + close_handle( async->thread->process, async->wait_handle ); + async->wait_handle = 0; + } + } + + /* The wait handle is preserved only when the status is STATUS_PENDING + * and async->blocking is set (i.e. we're going to block on it). */ + reply->handle = async->wait_handle; + } + else set_error( STATUS_ACCESS_DENIED ); + + release_object( &async->obj ); +} diff --git a/server/protocol.def b/server/protocol.def index 02e73047f9b..2c3b8dbc619 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 */ +(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 */
-- Sincerely, Jinoh Kang
On 1/27/22 13:22, Jinoh Kang wrote:
On 1/28/22 04:13, Jinoh Kang wrote:
On 1/28/22 04:05, Jinoh Kang wrote:
+ +/* Notify direct completion of async and close the wait handle */ +DECL_HANDLER(notify_async_direct_result) +{ + struct async *async = (struct async *)get_handle_obj( current->process, req->handle, 0, &async_ops ); + + if (!async) return; + + if (async->iosb && async->unknown_status && !async->pending && async->terminated) + { + /* Reactivate async. We call async_reselect() later. */ + async->terminated = 0; + + /* Set result for async_handoff(). */ + set_error( req->status ); + async->iosb->result = req->information; + + /* 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. + */ + async->unknown_status = 0; + async_handoff( async, NULL, 0 ); + + if (get_error() == STATUS_PENDING) + { + async_reselect( async ); + } + else if (NT_ERROR( get_error() )) + { + /* synchronous I/O failure: don't invoke callbacks, only dequeue it. */ + async_dequeue( async );
Note: we can't use async_set_result() here. async_handoff() leaves async->terminated as 0, and async_set_result() has "assert( async->terminated )".
Thus, reusing async_set_result() here requires either:
1. Setting async->pending to 0 so that async_handoff() will call async_terminate(). This is obviously incorrect since unwanted IOCP packets and APCs will fire on synchronous failure.
2. Not using async_handoff(). We have to copy a lot of code (particulary setting pending and direct_result flags) out of async_handoff() to do this.
Maybe just merge both branches (error/success) and force async_terminate() before calling async_set_result()?
Actually, I believe the right thing to do is just to set async->terminated = 1. In fact, I believe you should set async->terminated = 1 in the recv_socket request, otherwise async_waiting() will return 1 and the server will end up spinning and/or trying to wake the async. (Granted, that could be protected against by checking unknown_status, but...) There are a few different ways to look at this, but one way (and what I think makes the most sense to me) is that we're completing the async as if it had been alerted, much as we would from a kernel APC, except that we're also setting the initial status, which wasn't known yet. Hence we want to go through async_set_result(). Along these lines, if you also set async->alerted = 1, you get the STATUS_PENDING "restart the wait" behaviour for free. Which makes conceptual sense as well. I suppose this is why you mentioned direct_result in [1]—i.e. we could just async_terminate(STATUS_ALERTED) in recv_socket. I can see both approaches working, but I think that open-coding the relevant parts of async_terminate would be better than trying to get direct_result manipulation exactly right. Of course, I also haven't tried... [1] https://www.winehq.org/pipermail/wine-devel/2022-January/205774.html
On 1/28/22 08:32, Zebediah Figura (she/her) wrote:
On 1/27/22 13:22, Jinoh Kang wrote:
On 1/28/22 04:13, Jinoh Kang wrote:
On 1/28/22 04:05, Jinoh Kang wrote:
+ +/* Notify direct completion of async and close the wait handle */ +DECL_HANDLER(notify_async_direct_result) +{ + struct async *async = (struct async *)get_handle_obj( current->process, req->handle, 0, &async_ops ); + + if (!async) return; + + if (async->iosb && async->unknown_status && !async->pending && async->terminated) + { + /* Reactivate async. We call async_reselect() later. */ + async->terminated = 0; + + /* Set result for async_handoff(). */ + set_error( req->status ); + async->iosb->result = req->information; + + /* 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. + */ + async->unknown_status = 0; + async_handoff( async, NULL, 0 ); + + if (get_error() == STATUS_PENDING) + { + async_reselect( async ); + } + else if (NT_ERROR( get_error() )) + { + /* synchronous I/O failure: don't invoke callbacks, only dequeue it. */ + async_dequeue( async );
Note: we can't use async_set_result() here. async_handoff() leaves async->terminated as 0, and async_set_result() has "assert( async->terminated )".
Thus, reusing async_set_result() here requires either:
1. Setting async->pending to 0 so that async_handoff() will call async_terminate(). This is obviously incorrect since unwanted IOCP packets and APCs will fire on synchronous failure.
2. Not using async_handoff(). We have to copy a lot of code (particulary setting pending and direct_result flags) out of async_handoff() to do this.
Maybe just merge both branches (error/success) and force async_terminate() before calling async_set_result()?
Actually, I believe the right thing to do is just to set async->terminated = 1. In fact, I believe you should set async->terminated = 1 in the recv_socket request,
Yes, it's already done in async_handoff(). Note the precondition above:
+ if (async->iosb && async->unknown_status && !async->pending && async->terminated) + { + /* Reactivate async. We call async_reselect() later. */ + async->terminated = 0;
otherwise async_waiting() will return 1 and the server will end up spinning and/or trying to wake the async. (Granted, that could be protected against by checking unknown_status, but...)
There are a few different ways to look at this, but one way (and what I think makes the most sense to me) is that we're completing the async as if it had been alerted, much as we would from a kernel APC, except that we're also setting the initial status, which wasn't known yet. Hence we want to go through async_set_result().
Along these lines, if you also set async->alerted = 1, you get the STATUS_PENDING "restart the wait" behaviour for free. Which makes conceptual sense as well.
I'm honestly split off as to which side should take the responsibility of setting "async->alerted = 1", async_handoff() or notify_async_direct_result. I suppose I'm going with the former, since that's where STATUS_ALERTED is received. But then, shall the latter have "assert( async->alerted );"...?
I suppose this is why you mentioned direct_result in [1]—i.e. we could just async_terminate(STATUS_ALERTED) in recv_socket. I can see both approaches working, but I think that open-coding the relevant parts of async_terminate would be better than trying to get direct_result manipulation exactly right.
I agree. I hope we don't also have to also open code "async_handoff()", though.
Of course, I also haven't tried...
[1] https://www.winehq.org/pipermail/wine-devel/2022-January/205774.html
That said, the more I work with the async code, the more I learn to hate all those status bitfields... I think it's much better off merging as many states as possible into a single enum, so that the lifecycle is explicit (e.g. ASYNC_INITIAL -> ASYNC_QUEUED -> ASYNC_ALERTED -> ...) (I'm aware it was much worse before that, making the status field serve the dual role of async state and I/O status.) -- Sincerely, Jinoh Kang
Sorry for being slow to respond... On 1/27/22 22:17, Jinoh Kang wrote:
Note: we can't use async_set_result() here. async_handoff() leaves async->terminated as 0, and async_set_result() has "assert( async->terminated )".
Thus, reusing async_set_result() here requires either:
1. Setting async->pending to 0 so that async_handoff() will call async_terminate(). This is obviously incorrect since unwanted IOCP packets and APCs will fire on synchronous failure.
2. Not using async_handoff(). We have to copy a lot of code (particulary setting pending and direct_result flags) out of async_handoff() to do this.
Maybe just merge both branches (error/success) and force async_terminate() before calling async_set_result()?
Actually, I believe the right thing to do is just to set async->terminated = 1. In fact, I believe you should set async->terminated = 1 in the recv_socket request,
Yes, it's already done in async_handoff(). Note the precondition above:
+ if (async->iosb && async->unknown_status && !async->pending && async->terminated) + { + /* Reactivate async. We call async_reselect() later. */ + async->terminated = 0;
otherwise async_waiting() will return 1 and the server will end up spinning and/or trying to wake the async. (Granted, that could be protected against by checking unknown_status, but...)
There are a few different ways to look at this, but one way (and what I think makes the most sense to me) is that we're completing the async as if it had been alerted, much as we would from a kernel APC, except that we're also setting the initial status, which wasn't known yet. Hence we want to go through async_set_result().
Along these lines, if you also set async->alerted = 1, you get the STATUS_PENDING "restart the wait" behaviour for free. Which makes conceptual sense as well.
I'm honestly split off as to which side should take the responsibility of setting "async->alerted = 1", async_handoff() or notify_async_direct_result. I suppose I'm going with the former, since that's where STATUS_ALERTED is received.
I think that inasmuch as we're mimicking the usual "alerted" code path, it makes conceptual sense to set async->alerted = 1 in async_handoff().
But then, shall the latter have "assert( async->alerted );"...?
Well, you can't do that, exactly, because we can't crash no matter what input we get from the client. But you could check for async->alerted and return STATUS_INVALID_PARAMETER or something if it's not set.
I suppose this is why you mentioned direct_result in [1]—i.e. we could just async_terminate(STATUS_ALERTED) in recv_socket. I can see both approaches working, but I think that open-coding the relevant parts of async_terminate would be better than trying to get direct_result manipulation exactly right.
I agree. I hope we don't also have to also open code "async_handoff()", though.
Of course, I also haven't tried...
[1] https://www.winehq.org/pipermail/wine-devel/2022-January/205774.html
That said, the more I work with the async code, the more I learn to hate all those status bitfields... I think it's much better off merging as many states as possible into a single enum, so that the lifecycle is explicit (e.g. ASYNC_INITIAL -> ASYNC_QUEUED -> ASYNC_ALERTED -> ...) (I'm aware it was much worse before that, making the status field serve the dual role of async state and I/O status.)
Sure, I think that's something worth looking into.
On 2/2/22 13:55, Zebediah Figura wrote:
Sorry for being slow to respond...
No worries. Take your time.
I'm honestly split off as to which side should take the responsibility of setting "async->alerted = 1", async_handoff() or notify_async_direct_result. I suppose I'm going with the former, since that's where STATUS_ALERTED is received.
I think that inasmuch as we're mimicking the usual "alerted" code path, it makes conceptual sense to set async->alerted = 1 in async_handoff().
But then, shall the latter have "assert( async->alerted );"...?
Well, you can't do that, exactly, because we can't crash no matter what input we get from the client.
You're right. I just noted that in principle other assert()s are impossible to be triggered by the client.
But you could check for async->alerted and return STATUS_INVALID_PARAMETER or something if it's not set.
I did just that in the v3 of this patch series (but with STATUS_ACCESS_DENIED). In v4 I realised that it might be useful to let async be cancellable in the future (CancelSynchronousIo is not implemented yet), and the caller is not ready to handle errors from notify_async() anyway. So I decided to just return whatever the eventual status of the async is, whether alerted or not. Not sure how much it would complicate debugging though...
I suppose this is why you mentioned direct_result in [1]—i.e. we could just async_terminate(STATUS_ALERTED) in recv_socket. I can see both approaches working, but I think that open-coding the relevant parts of async_terminate would be better than trying to get direct_result manipulation exactly right.
I agree. I hope we don't also have to also open code "async_handoff()", though.
Of course, I also haven't tried...
[1] https://www.winehq.org/pipermail/wine-devel/2022-January/205774.html
That said, the more I work with the async code, the more I learn to hate all those status bitfields... I think it's much better off merging as many states as possible into a single enum, so that the lifecycle is explicit (e.g. ASYNC_INITIAL -> ASYNC_QUEUED -> ASYNC_ALERTED -> ...) (I'm aware it was much worse before that, making the status field serve the dual role of async state and I/O status.)
Sure, I think that's something worth looking into.
-- Sincerely, Jinoh Kang
Sorry for being slow to respond... On 1/27/22 22:17, Jinoh Kang wrote:
Note: we can't use async_set_result() here. async_handoff() leaves async->terminated as 0, and async_set_result() has "assert( async->terminated )".
Thus, reusing async_set_result() here requires either:
1. Setting async->pending to 0 so that async_handoff() will call async_terminate(). This is obviously incorrect since unwanted IOCP packets and APCs will fire on synchronous failure.
2. Not using async_handoff(). We have to copy a lot of code (particulary setting pending and direct_result flags) out of async_handoff() to do this.
Maybe just merge both branches (error/success) and force async_terminate() before calling async_set_result()?
Actually, I believe the right thing to do is just to set async->terminated = 1. In fact, I believe you should set async->terminated = 1 in the recv_socket request,
Yes, it's already done in async_handoff(). Note the precondition above:
+ if (async->iosb && async->unknown_status && !async->pending && async->terminated) + { + /* Reactivate async. We call async_reselect() later. */ + async->terminated = 0;
otherwise async_waiting() will return 1 and the server will end up spinning and/or trying to wake the async. (Granted, that could be protected against by checking unknown_status, but...)
There are a few different ways to look at this, but one way (and what I think makes the most sense to me) is that we're completing the async as if it had been alerted, much as we would from a kernel APC, except that we're also setting the initial status, which wasn't known yet. Hence we want to go through async_set_result().
Along these lines, if you also set async->alerted = 1, you get the STATUS_PENDING "restart the wait" behaviour for free. Which makes conceptual sense as well.
I'm honestly split off as to which side should take the responsibility of setting "async->alerted = 1", async_handoff() or notify_async_direct_result. I suppose I'm going with the former, since that's where STATUS_ALERTED is received.
I think that inasmuch as we're mimicking the usual "alerted" code path, it makes conceptual sense to set async->alerted = 1 in async_handoff().
But then, shall the latter have "assert( async->alerted );"...?
Well, you can't do that, exactly, because we can't crash no matter what input we get from the client. But you could check for async->alerted and return STATUS_INVALID_PARAMETER or something if it's not set.
I suppose this is why you mentioned direct_result in [1]—i.e. we could just async_terminate(STATUS_ALERTED) in recv_socket. I can see both approaches working, but I think that open-coding the relevant parts of async_terminate would be better than trying to get direct_result manipulation exactly right.
I agree. I hope we don't also have to also open code "async_handoff()", though.
Of course, I also haven't tried...
[1] https://www.winehq.org/pipermail/wine-devel/2022-January/205774.html
That said, the more I work with the async code, the more I learn to hate all those status bitfields... I think it's much better off merging as many states as possible into a single enum, so that the lifecycle is explicit (e.g. ASYNC_INITIAL -> ASYNC_QUEUED -> ASYNC_ALERTED -> ...) (I'm aware it was much worse before that, making the status field serve the dual role of async state and I/O status.)
Sure, I think that's something worth looking into.
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> --- 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..336f655e19e 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) { 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 2c3b8dbc619..72a7f4a85b5 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
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> --- 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 336f655e19e..a46e84616ec 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
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> --- 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 a46e84616ec..c6ab6c7646b 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 72a7f4a85b5..2de8bcd0a50 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
participants (3)
-
Jinoh Kang -
Zebediah Figura -
Zebediah Figura (she/her)