[PATCH v4 0/4] MR7797: ntdll: Wait for all pending operations to be cancelled in NtCancelIoFile[Ex]().
This fixes Trials Fusion often crashing when disconnecting a controller while there are more still connected. -- v4: hidclass: Set Status for pending IRPs of removed devices to STATUS_DEVICE_NOT_CONNECTED. ntdll/tests: Add more NtCancelIoFile[Ex]() tests. ntdll: Wait for all pending operations to be cancelled in NtCancelIoFile[Ex](). https://gitlab.winehq.org/wine/wine/-/merge_requests/7797
From: Matteo Bruni <mbruni(a)codeweavers.com> --- dlls/ntdll/unix/file.c | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index d7ab8b74734..679c2c44332 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -6421,19 +6421,16 @@ NTSTATUS WINAPI NtFlushBuffersFileEx( HANDLE handle, ULONG flags, void *params, } -/************************************************************************** - * NtCancelIoFile (NTDLL.@) - */ -NTSTATUS WINAPI NtCancelIoFile( HANDLE handle, IO_STATUS_BLOCK *io_status ) +static NTSTATUS cancel_io( HANDLE handle, IO_STATUS_BLOCK *io, IO_STATUS_BLOCK *io_status, + BOOL only_thread ) { unsigned int status; - TRACE( "%p %p\n", handle, io_status ); - SERVER_START_REQ( cancel_async ) { req->handle = wine_server_obj_handle( handle ); - req->only_thread = TRUE; + req->iosb = wine_server_client_ptr( io ); + req->only_thread = only_thread; if (!(status = wine_server_call( req ))) { io_status->Status = status; @@ -6446,28 +6443,25 @@ NTSTATUS WINAPI NtCancelIoFile( HANDLE handle, IO_STATUS_BLOCK *io_status ) } +/************************************************************************** + * NtCancelIoFile (NTDLL.@) + */ +NTSTATUS WINAPI NtCancelIoFile( HANDLE handle, IO_STATUS_BLOCK *io_status ) +{ + TRACE( "%p %p\n", handle, io_status ); + + return cancel_io( handle, NULL, io_status, TRUE ); +} + + /************************************************************************** * NtCancelIoFileEx (NTDLL.@) */ NTSTATUS WINAPI NtCancelIoFileEx( HANDLE handle, IO_STATUS_BLOCK *io, IO_STATUS_BLOCK *io_status ) { - unsigned int status; - TRACE( "%p %p %p\n", handle, io, io_status ); - SERVER_START_REQ( cancel_async ) - { - req->handle = wine_server_obj_handle( handle ); - req->iosb = wine_server_client_ptr( io ); - if (!(status = wine_server_call( req ))) - { - io_status->Status = status; - io_status->Information = 0; - } - } - SERVER_END_REQ; - - return status; + return cancel_io( handle, io, io_status, FALSE ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7797
From: Matteo Bruni <mbruni(a)codeweavers.com> --- dlls/ntdll/unix/file.c | 39 ++++++++++++++----- include/wine/server_protocol.h | 5 ++- server/async.c | 69 ++++++++++++++++++++++++++++++---- server/protocol.def | 3 ++ server/request_handlers.h | 2 + server/request_trace.h | 8 +++- 6 files changed, 107 insertions(+), 19 deletions(-) diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index 679c2c44332..a7ac1e124ee 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -6424,21 +6424,40 @@ NTSTATUS WINAPI NtFlushBuffersFileEx( HANDLE handle, ULONG flags, void *params, static NTSTATUS cancel_io( HANDLE handle, IO_STATUS_BLOCK *io, IO_STATUS_BLOCK *io_status, BOOL only_thread ) { - unsigned int status; + unsigned int status, count = 8; + HANDLE *handles; + int i; - SERVER_START_REQ( cancel_async ) + do { - req->handle = wine_server_obj_handle( handle ); - req->iosb = wine_server_client_ptr( io ); - req->only_thread = only_thread; - if (!(status = wine_server_call( req ))) + if (!(handles = malloc( count * sizeof(*handles) ))) return STATUS_NO_MEMORY; + + SERVER_START_REQ( cancel_async ) { - io_status->Status = status; - io_status->Information = 0; + obj_handle_t *server_handles = (obj_handle_t *)handles; + + req->handle = wine_server_obj_handle( handle ); + req->iosb = wine_server_client_ptr( io ); + req->only_thread = only_thread; + wine_server_set_reply( req, server_handles, count * sizeof(*server_handles) ); + if (!(status = wine_server_call( req ))) + { + count = reply->handles_size / sizeof(*server_handles); + for (i = count - 1; i >= 0; i--) handles[i] = wine_server_ptr_handle( server_handles[i] ); + io_status->Status = status; + io_status->Information = 0; + } + else if (status == STATUS_BUFFER_OVERFLOW) + { + count = reply->handles_size / sizeof(*server_handles); + free( handles ); + } } - } - SERVER_END_REQ; + SERVER_END_REQ; + } while (status == STATUS_BUFFER_OVERFLOW); + if (!status) NtWaitForMultipleObjects( count, handles, FALSE, TRUE, NULL ); + free( handles ); return status; } diff --git a/include/wine/server_protocol.h b/include/wine/server_protocol.h index 3cdc9375e2d..4a5075ea33f 100644 --- a/include/wine/server_protocol.h +++ b/include/wine/server_protocol.h @@ -3238,6 +3238,9 @@ struct cancel_async_request struct cancel_async_reply { struct reply_header __header; + data_size_t handles_size; + /* VARARG(handles,uints,handles_size); */ + char __pad_12[4]; }; @@ -6798,6 +6801,6 @@ union generic_reply struct set_keyboard_repeat_reply set_keyboard_repeat_reply; }; -#define SERVER_PROTOCOL_VERSION 863 +#define SERVER_PROTOCOL_VERSION 864 #endif /* __WINE_WINE_SERVER_PROTOCOL_H */ diff --git a/server/async.c b/server/async.c index d2d929c9709..c697a2e6a02 100644 --- a/server/async.c +++ b/server/async.c @@ -63,6 +63,7 @@ struct async unsigned int comp_flags; /* completion flags */ async_completion_callback completion_callback; /* callback to be called on completion */ void *completion_callback_private; /* argument to completion_callback */ + struct list canceled_entry; }; static void async_dump( struct object *obj, int verbose ); @@ -118,8 +119,8 @@ static void async_satisfied( struct object *obj, struct wait_queue_entry *entry struct async *async = (struct async *)obj; assert( obj->ops == &async_ops ); - /* we only return an async handle for asyncs created via create_request_async() */ - assert( async->iosb ); + if (!async->iosb) + return; if (async->direct_result) { @@ -574,7 +575,27 @@ int async_waiting( struct async_queue *queue ) return !async->terminated; } -static int cancel_async( struct process *process, struct object *obj, struct thread *thread, client_ptr_t iosb ) +static int cancel_count_async( struct process *process, struct object *obj, struct thread *thread, + client_ptr_t iosb ) +{ + struct async *async; + int count = 0; + + LIST_FOR_EACH_ENTRY( async, &process->asyncs, struct async, process_entry ) + { + if (async->terminated || async->canceled || async->is_system) continue; + if ((!obj || (get_fd_user( async->fd ) == obj)) && + (!thread || async->thread == thread) && + (!iosb || async->data.iosb == iosb)) + { + count++; + } + } + return count; +} + +static int cancel_async( struct process *process, struct object *obj, struct thread *thread, + client_ptr_t iosb, struct list *canceled_list ) { struct async *async; int woken = 0; @@ -592,7 +613,9 @@ restart: (!iosb || async->data.iosb == iosb)) { async->canceled = 1; + async->signaled = 0; fd_cancel_async( async->fd, async ); + list_add_tail( canceled_list, &async->canceled_entry ); woken++; goto restart; } @@ -815,12 +838,44 @@ DECL_HANDLER(cancel_async) { struct object *obj = get_handle_obj( current->process, req->handle, 0, NULL ); struct thread *thread = req->only_thread ? current : NULL; + struct list canceled_list = LIST_INIT(canceled_list); + unsigned int count = 0, capacity, i; + struct async *async, *next_async; + obj_handle_t *ptr; + + if (!obj) + return; + + count = cancel_count_async( current->process, obj, thread, req->iosb ); + if (!count && !thread) set_error( STATUS_NOT_FOUND ); + release_object( obj ); + capacity = get_reply_max_size() / sizeof(*ptr); - if (obj) + if (count) { - int count = cancel_async( current->process, obj, thread, req->iosb ); - if (!count && !thread) set_error( STATUS_NOT_FOUND ); - release_object( obj ); + if (count > capacity) + { + set_error( STATUS_BUFFER_OVERFLOW ); + reply->handles_size = count * sizeof(*ptr); + return; + } + count = cancel_async( current->process, obj, thread, req->iosb, &canceled_list ); + count = min( count, capacity ); + i = 0; + if ((ptr = set_reply_data_size( count * sizeof(*ptr) ))) + { + LIST_FOR_EACH_ENTRY_SAFE( async, next_async, &canceled_list, struct async, canceled_entry ) + { + if (i++ < capacity) + { + if (!async->wait_handle) + async->wait_handle = alloc_handle( current->process, async, SYNCHRONIZE, 0 ); + *ptr++ = async->wait_handle; + } + list_remove( &async->canceled_entry ); + } + reply->handles_size = count * sizeof(*ptr); + } } } diff --git a/server/protocol.def b/server/protocol.def index 63bb0111473..7cf330f2cf2 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -2431,6 +2431,9 @@ enum message_type obj_handle_t handle; /* handle to comm port, socket or file */ client_ptr_t iosb; /* I/O status block (NULL=all) */ int only_thread; /* cancel matching this thread */ +(a)REPLY + data_size_t handles_size; + VARARG(handles,uints,handles_size); /* handles list */ @END diff --git a/server/request_handlers.h b/server/request_handlers.h index 2afc9707869..4652f1808d9 100644 --- a/server/request_handlers.h +++ b/server/request_handlers.h @@ -1386,6 +1386,8 @@ C_ASSERT( offsetof(struct cancel_async_request, handle) == 12 ); C_ASSERT( offsetof(struct cancel_async_request, iosb) == 16 ); C_ASSERT( offsetof(struct cancel_async_request, only_thread) == 24 ); C_ASSERT( sizeof(struct cancel_async_request) == 32 ); +C_ASSERT( offsetof(struct cancel_async_reply, handles_size) == 8 ); +C_ASSERT( sizeof(struct cancel_async_reply) == 16 ); C_ASSERT( offsetof(struct get_async_result_request, user_arg) == 16 ); C_ASSERT( sizeof(struct get_async_result_request) == 24 ); C_ASSERT( sizeof(struct get_async_result_reply) == 8 ); diff --git a/server/request_trace.h b/server/request_trace.h index bd961791024..91154a3ce87 100644 --- a/server/request_trace.h +++ b/server/request_trace.h @@ -1552,6 +1552,12 @@ static void dump_cancel_async_request( const struct cancel_async_request *req ) fprintf( stderr, ", only_thread=%d", req->only_thread ); } +static void dump_cancel_async_reply( const struct cancel_async_reply *req ) +{ + fprintf( stderr, " handles_size=%u", req->handles_size ); + dump_varargs_uints( ", handles=", min( cur_size, req->handles_size )); +} + static void dump_get_async_result_request( const struct get_async_result_request *req ) { dump_uint64( " user_arg=", &req->user_arg ); @@ -3792,7 +3798,7 @@ static const dump_func reply_dumpers[REQ_NB_REQUESTS] = NULL, NULL, NULL, - NULL, + (dump_func)dump_cancel_async_reply, (dump_func)dump_get_async_result_reply, (dump_func)dump_set_async_direct_result_reply, (dump_func)dump_read_reply, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7797
From: Matteo Bruni <mbruni(a)codeweavers.com> --- dlls/ntdll/tests/pipe.c | 63 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 56 insertions(+), 7 deletions(-) diff --git a/dlls/ntdll/tests/pipe.c b/dlls/ntdll/tests/pipe.c index cf98d555455..36bb1f1194c 100644 --- a/dlls/ntdll/tests/pipe.c +++ b/dlls/ntdll/tests/pipe.c @@ -569,13 +569,36 @@ static void test_nonalertable(void) CloseHandle(hPipe); } -static void test_cancelio(void) +struct cancelio_ctx { - IO_STATUS_BLOCK iosb; + HANDLE pipe; + HANDLE event; + IO_STATUS_BLOCK *iosb; + BOOL null_iosb; +}; + +static DWORD WINAPI cancelioex_thread_func(void *arg) +{ + struct cancelio_ctx *ctx = arg; IO_STATUS_BLOCK cancel_sb; - HANDLE hEvent; - HANDLE hPipe; - NTSTATUS res; + NTSTATUS res, status; + + res = pNtCancelIoFileEx(ctx->pipe, ctx->null_iosb ? NULL : ctx->iosb, &cancel_sb); + status = ctx->iosb->Status; + ok(!res, "NtCancelIoFileEx returned %lx\n", res); + + ok(status == STATUS_CANCELLED, "Wrong iostatus %lx\n", status); + ok(WaitForSingleObject(ctx->event, 0) == 0, "hEvent not signaled\n"); + + return 0; +} + +static void test_cancelio(void) +{ + IO_STATUS_BLOCK cancel_sb, iosb; + HANDLE hEvent, hPipe, thread; + struct cancelio_ctx ctx; + NTSTATUS res, status; hEvent = CreateEventW(NULL, TRUE, FALSE, NULL); ok(hEvent != INVALID_HANDLE_VALUE, "can't create event, GetLastError: %lx\n", GetLastError()); @@ -589,9 +612,13 @@ static void test_cancelio(void) ok(res == STATUS_PENDING, "NtFsControlFile returned %lx\n", res); res = pNtCancelIoFile(hPipe, &cancel_sb); + /* Save Status first thing after the call. We want to make very unlikely + * that the kernel APC updating Status could be executed after the call + * but before peeking at Status here. */ + status = iosb.Status; ok(!res, "NtCancelIoFile returned %lx\n", res); - ok(iosb.Status == STATUS_CANCELLED, "Wrong iostatus %lx\n", iosb.Status); + ok(status == STATUS_CANCELLED, "Wrong iostatus %lx\n", status); ok(WaitForSingleObject(hEvent, 0) == 0, "hEvent not signaled\n"); ok(!ioapc_called, "IOAPC ran too early\n"); @@ -616,9 +643,10 @@ static void test_cancelio(void) ok(res == STATUS_PENDING, "NtFsControlFile returned %lx\n", res); res = pNtCancelIoFileEx(hPipe, &iosb, &cancel_sb); + status = iosb.Status; ok(!res, "NtCancelIoFileEx returned %lx\n", res); - ok(iosb.Status == STATUS_CANCELLED, "Wrong iostatus %lx\n", iosb.Status); + ok(status == STATUS_CANCELLED, "Wrong iostatus %lx\n", status); ok(WaitForSingleObject(hEvent, 0) == 0, "hEvent not signaled\n"); iosb.Status = 0xdeadbeef; @@ -626,6 +654,27 @@ static void test_cancelio(void) ok(res == STATUS_NOT_FOUND, "NtCancelIoFileEx returned %lx\n", res); ok(iosb.Status == 0xdeadbeef, "Wrong iostatus %lx\n", iosb.Status); + memset(&iosb, 0x55, sizeof(iosb)); + res = listen_pipe(hPipe, hEvent, &iosb, FALSE); + ok(res == STATUS_PENDING, "NtFsControlFile returned %lx\n", res); + + ctx.pipe = hPipe; + ctx.event = hEvent; + ctx.iosb = &iosb; + ctx.null_iosb = FALSE; + thread = CreateThread(NULL, 0, cancelioex_thread_func, &ctx, 0, NULL); + WaitForSingleObject(thread, INFINITE); + CloseHandle(thread); + + memset(&iosb, 0x55, sizeof(iosb)); + res = listen_pipe(hPipe, hEvent, &iosb, FALSE); + ok(res == STATUS_PENDING, "NtFsControlFile returned %lx\n", res); + + ctx.null_iosb = TRUE; + thread = CreateThread(NULL, 0, cancelioex_thread_func, &ctx, 0, NULL); + WaitForSingleObject(thread, INFINITE); + CloseHandle(thread); + CloseHandle(hPipe); } else -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7797
From: Matteo Bruni <mbruni(a)codeweavers.com> Also adjust xinput to handle that. --- dlls/hidclass.sys/device.c | 2 +- dlls/xinput1_3/main.c | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c index cb65391e443..852c8ebfbff 100644 --- a/dlls/hidclass.sys/device.c +++ b/dlls/hidclass.sys/device.c @@ -122,7 +122,7 @@ void hid_queue_remove_pending_irps( struct hid_queue *queue ) while ((irp = hid_queue_pop_irp( queue ))) { - irp->IoStatus.Status = STATUS_DELETE_PENDING; + irp->IoStatus.Status = STATUS_DEVICE_NOT_CONNECTED; IoCompleteRequest( irp, IO_NO_INCREMENT ); } } diff --git a/dlls/xinput1_3/main.c b/dlls/xinput1_3/main.c index 28f57e93bce..379a1d2f0b8 100644 --- a/dlls/xinput1_3/main.c +++ b/dlls/xinput1_3/main.c @@ -573,7 +573,11 @@ static void read_controller_state(struct xinput_controller *controller) if (!GetOverlappedResult(controller->device, &controller->hid.read_ovl, &read_len, TRUE)) { if (GetLastError() == ERROR_OPERATION_ABORTED) return; - if (GetLastError() == ERROR_ACCESS_DENIED || GetLastError() == ERROR_INVALID_HANDLE) controller_destroy(controller, TRUE); + if (GetLastError() == ERROR_ACCESS_DENIED || GetLastError() == ERROR_INVALID_HANDLE || + GetLastError() == ERROR_DEVICE_NOT_CONNECTED) + { + controller_destroy(controller, TRUE); + } else ERR("Failed to read input report, GetOverlappedResult failed with error %lu\n", GetLastError()); return; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7797
Rémi Bernon (@rbernon) commented about server/async.c:
+ count = min( count, capacity ); + i = 0; + if ((ptr = set_reply_data_size( count * sizeof(*ptr) ))) + { + LIST_FOR_EACH_ENTRY_SAFE( async, next_async, &canceled_list, struct async, canceled_entry ) + { + if (i++ < capacity) + { + if (!async->wait_handle) + async->wait_handle = alloc_handle( current->process, async, SYNCHRONIZE, 0 ); + *ptr++ = async->wait_handle; + } + list_remove( &async->canceled_entry ); + } + reply->handles_size = count * sizeof(*ptr); + } I think this could be reordered and you would not really need a list: Call `set_reply_data_size` first, and pass the pointer to cancel_async, which would allocate the wait handles if necessary and fill the array directly.
Another option for wait handle allocation is to move it to async creation unconditionally, which would be better for allocation failure handling, but that might be a waste of handles, so idk. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102164
Rémi Bernon (@rbernon) commented about server/async.c:
+ + count = cancel_count_async( current->process, obj, thread, req->iosb ); if (!count && !thread) set_error( STATUS_NOT_FOUND ); release_object( obj ); + capacity = get_reply_max_size() / sizeof(*ptr); + + if (count) + { + if (count > capacity) + { + set_error( STATUS_BUFFER_OVERFLOW ); + reply->handles_size = count * sizeof(*ptr); + return; + } + count = cancel_async( current->process, obj, thread, req->iosb, &canceled_list ); + count = min( count, capacity ); Do we actually need this? How can count change from the previous cancel_count_async call?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102165
Rémi Bernon (@rbernon) commented about server/async.c:
+ if (count) + { + if (count > capacity) + { + set_error( STATUS_BUFFER_OVERFLOW ); + reply->handles_size = count * sizeof(*ptr); + return; + } + count = cancel_async( current->process, obj, thread, req->iosb, &canceled_list ); + count = min( count, capacity ); + i = 0; + if ((ptr = set_reply_data_size( count * sizeof(*ptr) ))) + { + LIST_FOR_EACH_ENTRY_SAFE( async, next_async, &canceled_list, struct async, canceled_entry ) + { + if (i++ < capacity) Same thing, this doesn't look necessary as you already check `count > capacity` above.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102166
On Tue Apr 29 17:21:24 2025 +0000, Rémi Bernon wrote:
Do we actually need this? How can count change from the previous cancel_count_async call? As I understand it, it could change because of `async_reselect()` that's eventually called by `fd_cancel_async()` (see the comment in the `cancel_async()` function). Although IIUC that could only lower the count (if asyncs completed in the meantime), not increase it, which would allow some simplification.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102394
I think this could be reordered and you would not really need a list: Call `set_reply_data_size` first, and pass the pointer to cancel_async, which would allocate the wait handles if necessary and fill the array directly.
Indeed, nice catch! I didn't have the "counting" pass initially so I actually needed the list at that point, but not so much now. Updated locally.
Another option for wait handle allocation is to move it to async creation unconditionally, which would be better for allocation failure handling, but that might be a waste of handles, so idk.
Right, I thought it might be overkill in the most likely case where CancelIo() is never called. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102393
On Tue Apr 29 17:21:24 2025 +0000, Rémi Bernon wrote:
Same thing, this doesn't look necessary as you already check `count > capacity` above. Agreed, if I'm correct in my comment above this is not needed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102395
On Fri May 2 18:16:04 2025 +0000, Matteo Bruni wrote:
As I understand it, it could change because of `async_reselect()` that's eventually called by `fd_cancel_async()` (see the comment in the `cancel_async()` function). Although IIUC that could only lower the count (if asyncs completed in the meantime), not increase it, which would allow some simplification. Hmm... if the list can indeed change I'm thinking that the separate counting step isn't the right approach. I would suggest, instead of a cancelled flag, to keep asyncs in a dedicated list, as in https://gitlab.winehq.org/rbernon/wine/-/commits/mr/7797.
It also made me think that using wait_handle here makes things racy: if another thread waits on the async, gets it satisfied, and closes it, *after* we've returned the handle to the cancelling thread, but *before* that other thread has started waiting on it, the cancelling thread will try to wait on an invalid handle. So, in that branch I'm creating new handles for the cancelling thread, which then each need to be closed separately. Which then possibly shows that async might need to be done differently to avoid all these roundtrips, but I'm not knowledgeable enough there. I'm also not completely sure about putting all the cancelled asyncs in a process-wide list. Other threads cancelling async in the same process would wait on all the process previously cancelled asyncs, and maybe that would need to be done per-thread? Still, is it an issue if cancelled async are possibly short-lived? Then, there was a change in your code that cleared the async signaled flag before cancelling it, I'm not sure why it was needed and I removed it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102497
On Mon May 5 11:44:24 2025 +0000, Rémi Bernon wrote:
As I recall, I had to originally add it otherwise the `select` inside `NtCancelIoFile()` would be immediately satisfied i.e. it wouldn't wait for the actual async cancellation. Hmm... but if it's signaled already doesn't it mean that it has completed and shouldn't be cancelled? IIRC it's set to signaled immediately so that the original `select` can return `STATUS_PENDING`.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102508
As I recall, I had to originally add it otherwise the `select` inside `NtCancelIoFile()` would be immediately satisfied i.e. it wouldn't wait for the actual async cancellation.
Hmm... but if it's signaled already doesn't it mean that it has completed and shouldn't be cancelled? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102506
On Mon May 5 11:39:34 2025 +0000, Matteo Bruni wrote:
I'm also not completely sure about putting all the cancelled asyncs in a process-wide list. Other threads cancelling async in the same process would wait on all the process previously cancelled asyncs, and maybe that would need to be done per-thread? Still, is it an issue if cancelled async are possibly short-lived?
I don't think this one is an issue? asyncs are flagged immediately for cancellation and are then ignored for future `NtCancelIoFile()` calls. Ah, I misunderstood here, you were referring to the list you introduced in your version. I guess we could restore the check that makes sure not to cancel the same async twice. They wouldn't be cancelled twice: once cancelled, async go to the "cancelled_asyncs" list and aren't enumerated in future cancellations.
The issue I was a bit concerned about was that threads which cancel async would wait for every async in the "cancelled_asyncs" list, not just the ones it added there. But like you said it's probably not a big issue if it's only a transient state. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102505
I'm also not completely sure about putting all the cancelled asyncs in a process-wide list. Other threads cancelling async in the same process would wait on all the process previously cancelled asyncs, and maybe that would need to be done per-thread? Still, is it an issue if cancelled async are possibly short-lived?
I don't think this one is an issue? asyncs are flagged immediately for cancellation and are then ignored for future `NtCancelIoFile()` calls.
Ah, I misunderstood here, you were referring to the list you introduced in your version. I guess we could restore the check that makes sure not to cancel the same async twice. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102504
It also made me think that using wait_handle here makes things racy: if another thread waits on the async, gets it satisfied (which closes it), _after_ we've returned the handle to the cancelling thread, but _before_ the cancelling thread has started waiting on it, the cancelling thread will try to wait on an invalid handle.
I think you're right, we probably need a separate ad-hoc handle for cancellation.
I'm also not completely sure about putting all the cancelled asyncs in a process-wide list. Other threads cancelling async in the same process would wait on all the process previously cancelled asyncs, and maybe that would need to be done per-thread? Still, is it an issue if cancelled async are possibly short-lived?
I don't think this one is an issue? asyncs are flagged immediately for cancellation and are then ignored for future `NtCancelIoFile()` calls.
Then, there was a change in your code that cleared the async signaled flag before cancelling it, I'm not sure why it was needed and I removed it.
As I recall, I had to originally add it otherwise the `select` inside `NtCancelIoFile()` would be immediately satisfied i.e. it wouldn't wait for the actual async cancellation. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102503
IIRC it's set to signaled immediately so that the original `select` can return `STATUS_PENDING`.
Hmm, well I don't know enough about how asyncs are used but it seem suspicious though. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102510
The issue I was a bit concerned about was that threads which cancel async would wait for every async in the "cancelled_asyncs" list, not just the ones it added there. But like you said it's probably not a big issue if it's only a transient state.
Ah got it, yeah that doesn't seem too bad to me in practice. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102509
Well, it's not really about the `signaled` meaning, but about resetting it arbitrarily just so that the cancelling thread can wait on it.
After the async has been changed to one of the "signaled" state, it's probably not supposed to go back to a state where it's not signaled. Mapping this to your state machine it's not a valid state transition as it wasn't done anywhere.
This is probably a fair concern (with the caveat that nobody here seems to have a clear idea of all the intricacies involved :sweat_smile:). I don't know for sure that e.g. creating a separate event would be more proper. I think I remember starting with that, then switching to just creating a handle for the async if it's not around already. I don't recall anymore but it doesn't feel like I encountered hard-blockers with that, it just seemed simpler to do it like I ended up doing. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102531
On Mon May 5 14:07:03 2025 +0000, Jinoh Kang wrote:
I stand corrected. For your original question, maybe this bit in server/async.c is relevant? ``` /* FIXME: it would probably be nice to replace the "canceled" flag with a * single LIST_FOR_EACH_ENTRY_SAFE, but currently cancelling an async can * cause other asyncs to be removed via async_reselect() */ ``` That's what I was referring to in my very first reply to this comment.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102530
On Mon May 5 13:35:53 2025 +0000, Jinoh Kang wrote:
To be fair I have never seen anyone get async states right the first time, even I was bit by the haze that is the state flags. Maybe this is a strong argument that the async state machinery finally needs to be reworked. Well, it's not really about the `signaled` meaning, but about resetting it arbitrarily just so that the cancelling thread can wait on it.
After the async has been changed to one of the "signaled" state, it's probably not supposed to go back to a state where it's not signaled. Mapping this to your state machine it's not a valid state transition as it wasn't done anywhere. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102524
On Mon May 5 13:33:52 2025 +0000, Jinoh Kang wrote:
Hmm, well I don't know enough about how asyncs are used but it seem suspicious though. signaled probably does not mean what you think it means. For asynchronous non-blocking I/O, `signaled` must be set immediately to 1 so that the initial system call can return without blocking. The actual I/O completion delivery is done by signaling the *event* or posting to associated IOCP, not the async. I have a more-or-less comprehensive documentation about how async works at !6369. You can see the comprehensive list of scenarios when signaled is set to 1 here: https://gitlab.winehq.org/wine/wine/-/merge_requests/6369/diffs#9c0778468c57... To be fair I have never seen anyone get async states right the first time, even I was bit by the haze that is the state flags. Maybe this is a strong argument that the async state machinery finally needs to be reworked.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102523
Hmm, well I don't know enough about how asyncs are used but it seem suspicious though.
signaled probably does not mean what you think it means. For asynchronous non-blocking I/O, `signaled` must be set immediately to 1 so that the initial system call can return without blocking. The actual I/O completion delivery is done by signaling the *event* or posting to associated IOCP, not the async. I have a more-or-less comprehensive documentation about how async works at !6369. You can see the comprehensive list of scenarios when signaled is set to 1 here: https://gitlab.winehq.org/wine/wine/-/merge_requests/6369/diffs#9c0778468c57... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102521
On Mon May 5 13:44:36 2025 +0000, Rémi Bernon wrote:
Well, it's not really about the `signaled` meaning, but about resetting it arbitrarily just so that the cancelling thread can wait on it. After the async has been changed to one of the "signaled" state, it's probably not supposed to go back to a state where it's not signaled. Mapping this to your state machine it's not a valid state transition as it wasn't done anywhere. I stand corrected.
For your original question, maybe this bit in server/async.c is relevant? ``` /* FIXME: it would probably be nice to replace the "canceled" flag with a * single LIST_FOR_EACH_ENTRY_SAFE, but currently cancelling an async can * cause other asyncs to be removed via async_reselect() */ ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102526
On Mon May 5 14:41:33 2025 +0000, Matteo Bruni wrote:
Well, it's not really about the `signaled` meaning, but about resetting it arbitrarily just so that the cancelling thread can wait on it.
After the async has been changed to one of the "signaled" state, it's probably not supposed to go back to a state where it's not signaled. Mapping this to your state machine it's not a valid state transition as it wasn't done anywhere. This is probably a fair concern (with the caveat that nobody here seems to have a clear idea of all the intricacies involved :sweat_smile:). I don't know for sure that e.g. creating a separate event would be more proper. I think I remember starting with that, then switching to just creating a handle for the async if it's not around already. I don't recall anymore but it doesn't feel like I encountered hard-blockers with that, it just seemed simpler to do it like I ended up doing. It seems to me that before we can actually implement proper async cancellation, we will need to clarify what exactly is the semantics of the wait that follows the cancellation.
From @iamahuman state machine description, it looks like asyncs use "wait" operation for different things depending on the nature of the async. Some use it to "close" the async https://gitlab.winehq.org/wine/wine/-/merge_requests/6369/diffs#9c0778468c57..., others to "signal completion" (sic) https://gitlab.winehq.org/wine/wine/-/merge_requests/6369/diffs#9c0778468c57.... Is this what the cancelling thread is supposed to do on behalf of the thread normally handling the async? Using "wait" for these seems quite misleading to me.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102533
On Mon May 5 14:48:08 2025 +0000, Rémi Bernon wrote:
It seems to me that before we can actually implement proper async cancellation, we will need to clarify what exactly is the semantics of the wait that follows the cancellation. From @iamahuman state machine description, it looks like asyncs use "wait" operation for different things depending on the nature of the async. Some use it to "close" the async https://gitlab.winehq.org/wine/wine/-/merge_requests/6369/diffs#9c0778468c57..., others to "signal completion" (sic) https://gitlab.winehq.org/wine/wine/-/merge_requests/6369/diffs#9c0778468c57.... Is this what the cancelling thread is supposed to do on behalf of the thread normally handling the async? Using "wait" for these seems quite misleading to me. I'm probably missing a lot of context, but the very idea of returning a variably sized list for the client to wait isn't appealing to meeither.
Instead, we should probably define a new async op (via `create_request_async`) for the cancelling operation itself. Just a separate event isn't enough, since we need proper IOSB management for the cancelling operation itself (note that NtCnacelIoFile accepts *two* IOSBs.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102534
From @iamahuman state machine description, it looks like asyncs use "wait" operation for different things depending on the nature of the async. Some use it to "close" the async https://gitlab.winehq.org/wine/wine/-/merge_requests/6369/diffs#9c0778468c57..., others to "signal completion" (sic) https://gitlab.winehq.org/wine/wine/-/merge_requests/6369/diffs#9c0778468c57.... Is this what the cancelling thread is supposed to do on behalf of the thread normally handling the async? Using "wait" for these seems quite misleading to me.
If I had to guess, it's probably to "save" wineserver round trips. It's also possible that, back then, the resulting complexity was not well thought out. I have a few ideas as to how to stop wine abusing waits, but I'm hesitant to proceed before a refactor. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7797#note_102538
participants (4)
-
Jinoh Kang (＠iamahuman) -
Matteo Bruni
-
Matteo Bruni (＠Mystral)
-
Rémi Bernon