[PATCH 0/4] MR8592: ntdll: Make wait in wait_async() always non-alertable.
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/tests/pipe.c | 134 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 123 insertions(+), 11 deletions(-) diff --git a/dlls/ntdll/tests/pipe.c b/dlls/ntdll/tests/pipe.c index 14a6148a2c5..2bb6dedf093 100644 --- a/dlls/ntdll/tests/pipe.c +++ b/dlls/ntdll/tests/pipe.c @@ -1655,19 +1655,27 @@ struct blocking_thread_args enum { BLOCKING_THREAD_WRITE, BLOCKING_THREAD_READ, + BLOCKING_THREAD_USER_APC, + BLOCKING_THREAD_CANCEL_IO, BLOCKING_THREAD_QUIT } cmd; HANDLE client; HANDLE pipe; HANDLE event; + HANDLE io_thread; + unsigned int line; }; +#define blocking_thread_command(command) {ctx.cmd = command; ctx.line = __LINE__; SetEvent(ctx.wait);} + static DWORD WINAPI blocking_thread(void *arg) { struct blocking_thread_args *ctx = arg; static const char buf[] = "testdata"; char read_buf[32]; DWORD res, num_bytes; + IO_STATUS_BLOCK io; + NTSTATUS status; BOOL ret; for (;;) @@ -1675,6 +1683,7 @@ static DWORD WINAPI blocking_thread(void *arg) res = WaitForSingleObject(ctx->wait, 10000); ok(res == WAIT_OBJECT_0, "wait returned %lx\n", res); if (res != WAIT_OBJECT_0) break; + winetest_push_context("test line %d", ctx->line); switch(ctx->cmd) { case BLOCKING_THREAD_WRITE: Sleep(100); @@ -1698,11 +1707,32 @@ static DWORD WINAPI blocking_thread(void *arg) ok(ret, "WriteFile failed, error %lu\n", GetLastError()); ok(is_signaled(ctx->pipe), "pipe is not signaled\n"); break; + case BLOCKING_THREAD_USER_APC: + Sleep(100); + if(ctx->event) + ok(!is_signaled(ctx->event), "event is signaled\n"); + ok(!ioapc_called, "ioapc called\n"); + ok(!userapc_called, "userapc called.\n"); + ok(!is_signaled(ctx->client), "client is signaled\n"); + ok(is_signaled(ctx->pipe), "pipe is not signaled\n"); + pQueueUserAPC(userapc, ctx->io_thread, 0); + break; + case BLOCKING_THREAD_CANCEL_IO: + Sleep(100); + if(ctx->event) + ok(!is_signaled(ctx->event), "event is signaled\n"); + ok(!ioapc_called, "ioapc called\n"); + ok(!is_signaled(ctx->client), "client is signaled\n"); + ok(is_signaled(ctx->pipe), "pipe is not signaled\n"); + status = pNtCancelSynchronousIoFile(ctx->io_thread, NULL, &io); + ok(status == STATUS_SUCCESS, "NtCancelSynchronousIoFile failed, status %#lx.\n", status); + break; case BLOCKING_THREAD_QUIT: return 0; default: ok(0, "unvalid command\n"); } + winetest_pop_context(); SetEvent(ctx->done); } @@ -1723,6 +1753,8 @@ static void test_blocking(ULONG options) ctx.wait = CreateEventW(NULL, FALSE, FALSE, NULL); ctx.done = CreateEventW(NULL, FALSE, FALSE, NULL); + DuplicateHandle(GetCurrentProcess(), GetCurrentThread(), GetCurrentProcess(), &ctx.io_thread, 0, FALSE, + DUPLICATE_SAME_ACCESS); thread = CreateThread(NULL, 0, blocking_thread, &ctx, 0, 0); ok(thread != INVALID_HANDLE_VALUE, "can't create thread, GetLastError: %lx\n", GetLastError()); @@ -1743,9 +1775,8 @@ static void test_blocking(ULONG options) /* blocking read with no event nor APC */ ioapc_called = FALSE; memset(&io, 0xff, sizeof(io)); - ctx.cmd = BLOCKING_THREAD_WRITE; ctx.event = NULL; - SetEvent(ctx.wait); + blocking_thread_command(BLOCKING_THREAD_WRITE); status = NtReadFile(ctx.client, NULL, NULL, NULL, &io, read_buf, sizeof(read_buf), NULL, NULL); ok(status == STATUS_SUCCESS, "status = %lx\n", status); ok(io.Status == STATUS_SUCCESS, "Status = %lx\n", io.Status); @@ -1758,9 +1789,8 @@ static void test_blocking(ULONG options) /* blocking read with event and APC */ ioapc_called = FALSE; memset(&io, 0xff, sizeof(io)); - ctx.cmd = BLOCKING_THREAD_WRITE; ctx.event = CreateEventW(NULL, TRUE, TRUE, NULL); - SetEvent(ctx.wait); + blocking_thread_command(BLOCKING_THREAD_WRITE); status = NtReadFile(ctx.client, ctx.event, ioapc, &io, &io, read_buf, sizeof(read_buf), NULL, NULL); ok(status == STATUS_SUCCESS, "status = %lx\n", status); @@ -1772,12 +1802,14 @@ static void test_blocking(ULONG options) if (!(options & FILE_SYNCHRONOUS_IO_ALERT)) ok(!ioapc_called, "ioapc called\n"); + else + todo_wine ok(ioapc_called, "ioapc called\n"); SleepEx(0, TRUE); /* alertable wait state */ ok(ioapc_called, "ioapc not called\n"); res = WaitForSingleObject(ctx.done, 10000); ok(res == WAIT_OBJECT_0, "wait returned %lx\n", res); - ioapc_called = FALSE; + CloseHandle(ctx.event); ctx.event = NULL; @@ -1787,8 +1819,7 @@ static void test_blocking(ULONG options) ioapc_called = FALSE; memset(&io, 0xff, sizeof(io)); - ctx.cmd = BLOCKING_THREAD_READ; - SetEvent(ctx.wait); + blocking_thread_command(BLOCKING_THREAD_READ); status = NtFlushBuffersFile(ctx.client, &io); ok(status == STATUS_SUCCESS, "status = %lx\n", status); ok(io.Status == STATUS_SUCCESS, "Status = %lx\n", io.Status); @@ -1814,8 +1845,7 @@ static void test_blocking(ULONG options) ioapc_called = FALSE; memset(&io, 0xff, sizeof(io)); - ctx.cmd = BLOCKING_THREAD_READ; - SetEvent(ctx.wait); + blocking_thread_command(BLOCKING_THREAD_READ); status = NtFlushBuffersFile(ctx.client, &io); ok(status == STATUS_SUCCESS, "status = %lx\n", status); ok(io.Status == STATUS_SUCCESS, "Status = %lx\n", io.Status); @@ -1828,13 +1858,93 @@ static void test_blocking(ULONG options) CloseHandle(ctx.pipe); CloseHandle(ctx.client); - ctx.cmd = BLOCKING_THREAD_QUIT; - SetEvent(ctx.wait); + ctx.event = CreateEventW(NULL, TRUE, TRUE, NULL); + status = create_pipe(&ctx.pipe, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, + options); + ok(status == STATUS_SUCCESS, "NtCreateNamedPipeFile returned %lx\n", status); + pRtlInitUnicodeString(&name, testpipe_nt); + InitializeObjectAttributes( &attr, &name, OBJ_CASE_INSENSITIVE, 0, NULL ); + status = NtCreateFile(&ctx.client, SYNCHRONIZE | GENERIC_READ | GENERIC_WRITE, &attr, &io, + NULL, 0, FILE_SHARE_READ | FILE_SHARE_WRITE, FILE_OPEN, + options, NULL, 0 ); + ok(status == STATUS_SUCCESS, "NtCreateFile returned %lx\n", status); + if (options & FILE_SYNCHRONOUS_IO_ALERT) + { + /* Alertable IO interrupted with pre-queued user APC. */ + io.Status = 0xdeadbeef; + io.Information = 0xdeadbeef; + pQueueUserAPC(userapc, GetCurrentThread(), 0); + userapc_called = FALSE; + ioapc_called = FALSE; + status = NtReadFile(ctx.client, ctx.event, ioapc, &io, &io, read_buf, + sizeof(read_buf), NULL, NULL); + todo_wine ok(status == STATUS_CANCELLED, "status = %lx\n", status); + todo_wine ok(io.Status == STATUS_CANCELLED || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + "Status = %lx\n", io.Status); + todo_wine ok(io.Information == 0 || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + "Information = %Iu\n", io.Information); + todo_wine ok(is_signaled(ctx.event) || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + "event is not signaled\n"); + ok(!ioapc_called, "ioapc called\n"); + ok(userapc_called, "user apc is not called.\n"); + SleepEx(0, TRUE); + ok(!ioapc_called, "ioapc called\n"); + + /* Alertable IO interrupted with user APC during wait. */ + io.Status = 0xdeadbeef; + io.Information = 0xdeadbeef; + userapc_called = FALSE; + ioapc_called = FALSE; + blocking_thread_command(BLOCKING_THREAD_USER_APC); + status = NtReadFile(ctx.client, ctx.event, ioapc, &io, &io, read_buf, + sizeof(read_buf), NULL, NULL); + todo_wine ok(status == STATUS_CANCELLED, "status = %lx\n", status); + todo_wine ok(io.Status == STATUS_CANCELLED || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + "Status = %lx\n", io.Status); + todo_wine ok(io.Information == 0 || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + "Information = %Iu\n", io.Information); + todo_wine ok(is_signaled(ctx.event) || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + "event is not signaled\n"); + ok(!ioapc_called, "ioapc called\n"); + ok(userapc_called, "user apc is not called.\n"); + SleepEx(0, TRUE); + ok(!ioapc_called, "ioapc called\n"); + } + + /* IO interrupted with NtCancelSynchronousIoFile(). */ + io.Status = 0xdeadbeef; + io.Information = 0xdeadbeef; + userapc_called = FALSE; + ioapc_called = FALSE; + blocking_thread_command(BLOCKING_THREAD_CANCEL_IO); + status = NtReadFile(ctx.client, ctx.event, ioapc, &io, &io, read_buf, + sizeof(read_buf), NULL, NULL); + ok(status == STATUS_CANCELLED, "status = %lx\n", status); + todo_wine ok(io.Status == STATUS_CANCELLED || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + "Status = %lx\n", io.Status); + todo_wine ok(io.Information == 0 || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + "Information = %Iu\n", io.Information); + ok(is_signaled(ctx.event) || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + "event is not signaled\n"); + ok(!ioapc_called, "ioapc called\n"); + ok(!userapc_called, "user apc is not called.\n"); + SleepEx(0, TRUE); + todo_wine ok(!ioapc_called, "ioapc called\n"); + + ioapc_called = FALSE; + CloseHandle(ctx.event); + ctx.event = NULL; + + CloseHandle(ctx.pipe); + CloseHandle(ctx.client); + + blocking_thread_command(BLOCKING_THREAD_QUIT); res = WaitForSingleObject(thread, 10000); ok(res == WAIT_OBJECT_0, "wait returned %lx\n", res); CloseHandle(ctx.wait); CloseHandle(ctx.done); + CloseHandle(ctx.io_thread); CloseHandle(thread); } @@ -3118,7 +3228,9 @@ START_TEST(pipe) trace("starting blocking tests\n"); test_blocking(FILE_SYNCHRONOUS_IO_NONALERT); + winetest_push_context("aletrable"); test_blocking(FILE_SYNCHRONOUS_IO_ALERT); + winetest_pop_context(); trace("starting FILE_PIPE_INFORMATION tests\n"); test_filepipeinfo(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8592
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/unix/server.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index 3969c2bb898..e9d11f12e42 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -851,15 +851,24 @@ NTSTATUS WINAPI NtContinueEx( CONTEXT *context, KCONTINUE_ARGUMENT *args ) /*********************************************************************** - * NtTestAlert (NTDLL.@) + * test_alert_with_status */ -NTSTATUS WINAPI NtTestAlert(void) +static void test_alert_with_status( NTSTATUS ret_status ) { struct user_apc apc; NTSTATUS status; status = server_select( NULL, 0, SELECT_INTERRUPTIBLE | SELECT_ALERTABLE, 0, NULL, &apc ); - if (status == STATUS_USER_APC) invoke_user_apc( NULL, &apc, STATUS_SUCCESS ); + if (status == STATUS_USER_APC) invoke_user_apc( NULL, &apc, ret_status ); +} + + +/*********************************************************************** + * NtTestAlert (NTDLL.@) + */ +NTSTATUS WINAPI NtTestAlert(void) +{ + test_alert_with_status( STATUS_SUCCESS ); return STATUS_SUCCESS; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8592
From: Paul Gofman <pgofman(a)codeweavers.com> Cancel async and signal async object on user APC instead. --- dlls/ntdll/tests/pipe.c | 20 ++++++++++---------- dlls/ntdll/unix/server.c | 2 +- dlls/ntdll/unix/unix_private.h | 8 +++++++- server/async.c | 28 +++++++++++++++++++++++++--- server/fd.c | 6 ++++++ server/file.h | 2 ++ server/thread.c | 1 + 7 files changed, 52 insertions(+), 15 deletions(-) diff --git a/dlls/ntdll/tests/pipe.c b/dlls/ntdll/tests/pipe.c index 2bb6dedf093..6af05449ecf 100644 --- a/dlls/ntdll/tests/pipe.c +++ b/dlls/ntdll/tests/pipe.c @@ -477,7 +477,7 @@ static void test_alertable(void) ok(ret, "can't queue user apc, GetLastError: %lx\n", GetLastError()); res = listen_pipe(hPipe, hEvent, &iosb, TRUE); - todo_wine ok(res == STATUS_CANCELLED, "NtFsControlFile returned %lx\n", res); + ok(res == STATUS_CANCELLED, "NtFsControlFile returned %lx\n", res); ok(userapc_called, "user apc didn't run\n"); ok(iosb.Status == 0x55555555 || iosb.Status == STATUS_CANCELLED, "iosb.Status got changed to %lx\n", iosb.Status); @@ -491,7 +491,7 @@ static void test_alertable(void) /* wine_todo: the earlier NtFsControlFile call gets cancelled after the pipe gets set into listen state instead of before, so this NtFsControlFile will fail STATUS_INVALID_HANDLE */ res = listen_pipe(hPipe, hEvent, &iosb, TRUE); - todo_wine ok(res == STATUS_CANCELLED, "NtFsControlFile returned %lx\n", res); + ok(res == STATUS_CANCELLED, "NtFsControlFile returned %lx\n", res); ok(userapc_called, "user apc didn't run\n"); ok(iosb.Status == 0x55555555 || iosb.Status == STATUS_CANCELLED, "iosb.Status got changed to %lx\n", iosb.Status); @@ -1803,7 +1803,7 @@ static void test_blocking(ULONG options) if (!(options & FILE_SYNCHRONOUS_IO_ALERT)) ok(!ioapc_called, "ioapc called\n"); else - todo_wine ok(ioapc_called, "ioapc called\n"); + ok(ioapc_called, "ioapc called\n"); SleepEx(0, TRUE); /* alertable wait state */ ok(ioapc_called, "ioapc not called\n"); @@ -1878,12 +1878,12 @@ static void test_blocking(ULONG options) ioapc_called = FALSE; status = NtReadFile(ctx.client, ctx.event, ioapc, &io, &io, read_buf, sizeof(read_buf), NULL, NULL); - todo_wine ok(status == STATUS_CANCELLED, "status = %lx\n", status); - todo_wine ok(io.Status == STATUS_CANCELLED || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + ok(status == STATUS_CANCELLED, "status = %lx\n", status); + ok(io.Status == STATUS_CANCELLED || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, "Status = %lx\n", io.Status); - todo_wine ok(io.Information == 0 || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + ok(io.Information == 0 || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, "Information = %Iu\n", io.Information); - todo_wine ok(is_signaled(ctx.event) || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + ok(is_signaled(ctx.event) || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, "event is not signaled\n"); ok(!ioapc_called, "ioapc called\n"); ok(userapc_called, "user apc is not called.\n"); @@ -1898,12 +1898,12 @@ static void test_blocking(ULONG options) blocking_thread_command(BLOCKING_THREAD_USER_APC); status = NtReadFile(ctx.client, ctx.event, ioapc, &io, &io, read_buf, sizeof(read_buf), NULL, NULL); - todo_wine ok(status == STATUS_CANCELLED, "status = %lx\n", status); + ok(status == STATUS_CANCELLED, "status = %lx\n", status); todo_wine ok(io.Status == STATUS_CANCELLED || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, "Status = %lx\n", io.Status); todo_wine ok(io.Information == 0 || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, "Information = %Iu\n", io.Information); - todo_wine ok(is_signaled(ctx.event) || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + ok(is_signaled(ctx.event) || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, "event is not signaled\n"); ok(!ioapc_called, "ioapc called\n"); ok(userapc_called, "user apc is not called.\n"); @@ -1929,7 +1929,7 @@ static void test_blocking(ULONG options) ok(!ioapc_called, "ioapc called\n"); ok(!userapc_called, "user apc is not called.\n"); SleepEx(0, TRUE); - todo_wine ok(!ioapc_called, "ioapc called\n"); + ok(!ioapc_called, "ioapc called\n"); ioapc_called = FALSE; CloseHandle(ctx.event); diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index e9d11f12e42..17531b2a1ca 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -853,7 +853,7 @@ NTSTATUS WINAPI NtContinueEx( CONTEXT *context, KCONTINUE_ARGUMENT *args ) /*********************************************************************** * test_alert_with_status */ -static void test_alert_with_status( NTSTATUS ret_status ) +void test_alert_with_status( NTSTATUS ret_status ) { struct user_apc apc; NTSTATUS status; diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h index fa38f4b87ca..74fb95644ed 100644 --- a/dlls/ntdll/unix/unix_private.h +++ b/dlls/ntdll/unix/unix_private.h @@ -393,6 +393,8 @@ extern NTSTATUS call_user_apc_dispatcher( CONTEXT *context_ptr, ULONG_PTR arg1, extern NTSTATUS call_user_exception_dispatcher( EXCEPTION_RECORD *rec, CONTEXT *context ); extern void call_raise_user_exception_dispatcher(void); +extern void test_alert_with_status( NTSTATUS status ); + #define IMAGE_DLLCHARACTERISTICS_PREFER_NATIVE 0x0010 /* Wine extension */ #define TICKSPERSEC 10000000 @@ -467,7 +469,11 @@ static inline struct async_data server_async( HANDLE handle, struct async_fileio static inline NTSTATUS wait_async( HANDLE handle, BOOL alertable ) { - return NtWaitForSingleObject( handle, alertable, NULL ); + NTSTATUS ret; + + ret = NtWaitForSingleObject( handle, FALSE, NULL ); + if (alertable) test_alert_with_status( ret ); + return ret; } static inline BOOL in_wow64_call(void) diff --git a/server/async.c b/server/async.c index 1ed241dcb65..38b870e2f10 100644 --- a/server/async.c +++ b/server/async.c @@ -396,6 +396,12 @@ obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_ return 0; } + if (async->blocking && is_fd_wait_alertable( async->fd ) && !list_empty( &async->thread->user_apc )) + { + async->canceled = 1; + async_terminate( async, STATUS_CANCELLED ); + } + if (async->iosb->status != STATUS_PENDING) { if (result) *result = async->iosb->result; @@ -512,9 +518,9 @@ void async_set_result( struct object *obj, unsigned int status, apc_param_t tota /* 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->pending || async->canceled || !NT_ERROR( status )) { - if (async->data.apc) + if (async->data.apc && !(async->blocking && async->canceled)) { union apc_call data; memset( &data, 0, sizeof(data) ); @@ -525,7 +531,7 @@ void async_set_result( struct object *obj, unsigned int status, apc_param_t tota data.user.args[2] = 0; thread_queue_apc( NULL, async->thread, NULL, &data ); } - else if (async->data.apc_context && (async->pending || + else if (!async->data.apc && 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 ); @@ -675,6 +681,22 @@ restart: } } +void cancel_alerted_thread_async( struct thread *thread ) +{ + struct async *async; + + LIST_FOR_EACH_ENTRY( async, &thread->process->asyncs, struct async, process_entry ) + { + if (async->terminated || async->canceled) continue; + if (async->blocking && async->thread == thread && async->fd && is_fd_wait_alertable( async->fd )) + { + async->canceled = 1; + fd_cancel_async( async->fd, async ); + return; + } + } +} + /* wake up async operations on the queue */ void async_wake_up( struct async_queue *queue, unsigned int status ) { diff --git a/server/fd.c b/server/fd.c index 663f497b7a9..997e66ca517 100644 --- a/server/fd.c +++ b/server/fd.c @@ -2139,6 +2139,12 @@ int is_fd_overlapped( struct fd *fd ) return !(fd->options & (FILE_SYNCHRONOUS_IO_ALERT | FILE_SYNCHRONOUS_IO_NONALERT)); } +/* check if fd is in synchronous alert mode */ +int is_fd_wait_alertable( struct fd *fd ) +{ + return fd->options & FILE_SYNCHRONOUS_IO_ALERT; +} + /* retrieve the unix fd for an object */ int get_unix_fd( struct fd *fd ) { diff --git a/server/file.h b/server/file.h index 567194bf00a..972e4f54bb4 100644 --- a/server/file.h +++ b/server/file.h @@ -94,6 +94,7 @@ extern void set_fd_user( struct fd *fd, const struct fd_ops *ops, struct object extern unsigned int get_fd_options( struct fd *fd ); extern unsigned int get_fd_comp_flags( struct fd *fd ); extern int is_fd_overlapped( struct fd *fd ); +extern int is_fd_wait_alertable( struct fd *fd ); extern int get_unix_fd( struct fd *fd ); extern client_ptr_t get_fd_map_address( struct fd *fd ); extern void set_fd_map_address( struct fd *fd, client_ptr_t addr, mem_size_t size ); @@ -278,6 +279,7 @@ extern void fd_copy_completion( struct fd *src, struct fd *dst ); extern struct iosb *async_get_iosb( struct async *async ); extern struct thread *async_get_thread( struct async *async ); extern struct async *find_pending_async( struct async_queue *queue ); +extern void cancel_alerted_thread_async( struct thread *thread ); extern void cancel_process_asyncs( struct process *process ); extern void cancel_terminating_thread_asyncs( struct thread *thread ); extern int async_close_obj_handle( struct object *obj, struct process *process, obj_handle_t handle ); diff --git a/server/thread.c b/server/thread.c index 05ec6a4ec00..65feb5ffbb8 100644 --- a/server/thread.c +++ b/server/thread.c @@ -2073,6 +2073,7 @@ DECL_HANDLER(queue_apc) if (thread) { if (!queue_apc( NULL, thread, apc )) set_error( STATUS_UNSUCCESSFUL ); + else if (apc->call.type == APC_USER) cancel_alerted_thread_async( thread ); release_object( thread ); } else if (process) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8592
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/tests/pipe.c | 8 ++++---- server/async.c | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/dlls/ntdll/tests/pipe.c b/dlls/ntdll/tests/pipe.c index 6af05449ecf..f6324dc1f31 100644 --- a/dlls/ntdll/tests/pipe.c +++ b/dlls/ntdll/tests/pipe.c @@ -1899,9 +1899,9 @@ static void test_blocking(ULONG options) status = NtReadFile(ctx.client, ctx.event, ioapc, &io, &io, read_buf, sizeof(read_buf), NULL, NULL); ok(status == STATUS_CANCELLED, "status = %lx\n", status); - todo_wine ok(io.Status == STATUS_CANCELLED || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + ok(io.Status == STATUS_CANCELLED || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, "Status = %lx\n", io.Status); - todo_wine ok(io.Information == 0 || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + ok(io.Information == 0 || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, "Information = %Iu\n", io.Information); ok(is_signaled(ctx.event) || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, "event is not signaled\n"); @@ -1920,9 +1920,9 @@ static void test_blocking(ULONG options) status = NtReadFile(ctx.client, ctx.event, ioapc, &io, &io, read_buf, sizeof(read_buf), NULL, NULL); ok(status == STATUS_CANCELLED, "status = %lx\n", status); - todo_wine ok(io.Status == STATUS_CANCELLED || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + ok(io.Status == STATUS_CANCELLED || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, "Status = %lx\n", io.Status); - todo_wine ok(io.Information == 0 || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + ok(io.Information == 0 || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, "Information = %Iu\n", io.Information); ok(is_signaled(ctx.event) || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, "event is not signaled\n"); diff --git a/server/async.c b/server/async.c index 38b870e2f10..c759190105f 100644 --- a/server/async.c +++ b/server/async.c @@ -192,7 +192,7 @@ void async_terminate( struct async *async, unsigned int status ) * 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)) + if (NT_ERROR( status ) && !async->canceled && (!is_fd_overlapped( async->fd ) || !async->pending)) data.async_io.sb = 0; else data.async_io.sb = async->data.iosb; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8592
While working on https://gitlab.winehq.org/wine/wine/-/merge_requests/8572 I discovered that Windows, unlike our implementation, prioritizes user APC presence over satisfied wait conditions. Trying to fix that uncovered a couple of places covered by the tests which depend on the present WaitFor behaviour, one of those is the alertable synchronous IO. While looking into this part turned out alertable synchronous IO has more issues unrelated to this user APC precedence: 1. Currently wait_async() performs alertable wait for FILE_SYNCHRONOUS_IO_ALERT, and if user APC is queued that exits IO function at once. While not yet completed async is left behind and will write IOSB to then-random memory location once it is finally finished. Also, we return STATUS_USER_APC from NtWaitFor... performed in wait_async(), Windows instead returns STATUS_CANCELLED (across all the Windows versions), indicating that async IO was canceled first. That works the same way with NtCancelSynchronousIoFile. That is unrelated to fixing NtWaitFor.. 2. With NtWaitFor... preferring user APC result, alertable wait in wait_async() gets interrupted with user IO completion APC queued by us (if the APC is passed to IO function) while completing APC, so wait_async() ends with STATUS_USER_APC even if there was no side user APC queued. 3. On Windows IO completion user APC (queued by system on async completion) with alertable blocking IO file is executed on IO function exit, as also shown in my added tests. That is not happening now, because wait resolves first by signaled async and there is nothing to process the queued APC. 4. Up to date Windows 11 also sets IOSB on canceled sync read (both alertable and not). That's not only with the read interrupted by APC but also for NtCancelSynchronousIoFile. This looks like new behaviour, none of Testbot machines fill IOSB in my added tests. But my local Win11 24H2 does. That behaviour was on Wine was prevented by the check in async_terminate() introduced by commit 5af74129bd5d5f5c37ba25393da74c077fcb4676. I checked kernel32/tests/volume.c on my Win11 24H2 and confirmed that the tests still succeed on it. SO it doesn't look like the behaviour changed for all the error return cases, but it is now the case for blocking canceled asyncs at least. This part is concerned in the last patch "server: Set IOSB for cancelled asyncs in async_terminate()." The general idea behind fixing the most part of it (pp1-3, while p. 4 is mostly independent) is to get rid of alertable wait in wait_async(). It looks straightforward to me that we'd better manage special async object signal state properly, so it is only signaled once APC is concluded one or another way. While if that is alertable IO and user APC is present we should signal the async object; while wait_async() on the client can check for user APCs on exit, on the way fixing p. 3. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8592#note_110251
The tests failures are unrelated, I am reproducing locally and that regressed with https://gitlab.winehq.org/wine/wine/-/merge_requests/8559 (also commented there). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8592#note_110256
1/4: I think maybe we should also test a pre-queued (different) I/O APC, if that's not already tested. I will bet money that it behaves like the pre-queued user APC case, but... ``` + /* Alertable IO interrupted with pre-queued user APC. */ + io.Status = 0xdeadbeef; + io.Information = 0xdeadbeef; + pQueueUserAPC(userapc, GetCurrentThread(), 0); + userapc_called = FALSE; + ioapc_called = FALSE; + status = NtReadFile(ctx.client, ctx.event, ioapc, &io, &io, read_buf, + sizeof(read_buf), NULL, NULL); + todo_wine ok(status == STATUS_CANCELLED, "status = %lx\n", status); + todo_wine ok(io.Status == STATUS_CANCELLED || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + "Status = %lx\n", io.Status); + todo_wine ok(io.Information == 0 || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + "Information = %Iu\n", io.Information); + todo_wine ok(is_signaled(ctx.event) || broken(io.Status == 0xdeadbeef) /* Before Win11 24H2 */, + "event is not signaled\n"); + ok(!ioapc_called, "ioapc called\n"); + ok(userapc_called, "user apc is not called.\n"); + SleepEx(0, TRUE); + ok(!ioapc_called, "ioapc called\n"); ``` Probably worth checking that the event isn't signaled here either. (At least, I assume it isn't, if there's no APC queued...) ``` + winetest_push_context("aletrable"); ``` Typo here. In 3/4: ``` + if (async->blocking && async->thread == thread && async->fd && is_fd_wait_alertable( async->fd )) ``` I might be missing some way you've already accounted for this, but FILE_SYNCHRONOUS_IO_ALERT doesn't actually apply to all functions. Specifically, it doesn't apply to "always blocking" functions like NtFlushBuffersFile(). At least, that's what our current code has, though I'm not quite sure if it's adequately tested. Note that that's controlled on the server side via the "force_blocking" argument to async_handoff(). ``` + if (async->blocking && is_fd_wait_alertable( async->fd ) && !list_empty( &async->thread->user_apc )) + { + async->canceled = 1; + async_terminate( async, STATUS_CANCELLED ); + } ``` Not fd_cancel_async() here? ``` /* 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->pending || async->canceled || !NT_ERROR( status )) ``` Wait, what? This looks like a separate thing, and I'm probably missing something, but I don't see where it's justified by the tests. If it's correct can we please split it into its own patch? More generally some of the parts of this patch could be split anyway, e.g. checking for APCs in async_handoff() could be a separate patch. I don't know if it's necessary but it wouldn't hurt. 4/4: ``` - if (NT_ERROR( status ) && (!is_fd_overlapped( async->fd ) || !async->pending)) + if (NT_ERROR( status ) && !async->canceled && (!is_fd_overlapped( async->fd ) || !async->pending)) ``` This makes sense once you know the context, but I feel like without the rest of the context in this patch it's rather confusing—how can an async be cancelled when you haven't started waiting on it yet? So I'd probably add a bit to the above comment to answer that question. I'd recommend something like "an async can be cancelled without being pending in the case that the file has FILE_SYNCHRONOUS_IO_ALERT and there was already a user APC queued. Windows won't signal completion in this case, but Windows 11 will fill the IOSB regardless." -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8592#note_110326
I think maybe we should also test a pre-queued (different) I/O APC, if that's not already tested. I will bet money that it behaves like the pre-queued user APC case, but...
Probably worth checking that the event isn't signaled here either. (At least, I assume it isn't, if there's no APC queued...)
Sure, I will add the tests.
I might be missing some way you've already accounted for this, but FILE_SYNCHRONOUS_IO_ALERT doesn't actually apply to all functions. Specifically, it doesn't apply to "always blocking" functions like NtFlushBuffersFile(). At least, that's what our current code has, though I'm not quite sure if it's adequately tested.
Yes, I will add the test for NtFlushBuffersFile(), might be that it will trigger some change.
Not fd_cancel_async() here? Yes, should be that, I already checked that it works.
``` /* 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->pending || async->canceled || !NT_ERROR( status )) ```
Wait, what? This looks like a separate thing, and I'm probably missing something, but I don't see where it's justified by the tests. If it's correct can we please split it into its own patch?
Removing '|| async->canceled' from the final code breaks two tests at least in the same pipe.c: ``` pipe.c:1886: Test failed: aletrable: event is not signaled pipe.c:3235: starting FILE_PIPE_INFORMATION tests pipe.c:3241: starting alertable tests pipe.c:484: Test failed: hEvent signaled ``` Initially I had that as a separate patch after but that resulted in the failures in "test_alertable()" after "ntdll: Make wait in wait_async() always non-alertable.", so I squashed those changes.
More generally some of the parts of this patch could be split anyway, e.g. checking for APCs in async_handoff() could be a separate patch. I don't know if it's necessary but it wouldn't hurt.
That would need adding a part of the test later or that hangs (instead of having the test at once and then shuffling todo's to indicate which parts affect what). But if you think it is better I can do that of course. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8592#note_110327
Wait, what?
While yes, it looks weird. Event is signaled but IO completion user APC is not delivered. But that is consistent in tests across Windows versions, older ones which don't fill IOSB under tested conditions don't deliver IO apc, same as up to date Win11. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8592#note_110328
Probably worth checking that the event isn't signaled here either. (At least, I assume it isn't, if there's no APC queued...)
Sure, I will add the tests.
Ugh, I'm blind, it's already tested, and the event *is* signaled. That's literally what you were demonstrating with that hunk that looked wrong to me. Which suddenly doesn't look wrong anymore. Well, there was no way I was getting through this without an "I can't read" moment. ``` - if (async->data.apc) + if (async->data.apc && !(async->blocking && async->canceled)) { union apc_call data; memset( &data, 0, sizeof(data) ); @@ -525,7 +531,7 @@ void async_set_result( struct object *obj, unsigned int status, apc_param_t tota data.user.args[2] = 0; thread_queue_apc( NULL, async->thread, NULL, &data ); } - else if (async->data.apc_context && (async->pending || + else if (!async->data.apc && async->data.apc_context && (async->pending || ``` It'd probably be easier to read if you nest the "if !(blocking && canceled)" part inside the first if. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8592#note_110329
On Fri Jul 18 20:12:00 2025 +0000, Elizabeth Figura wrote:
Probably worth checking that the event isn't signaled here either. (At least, I assume it isn't, if there's no APC queued...)
Sure, I will add the tests. Ugh, I'm blind, it's already tested, and the event *is* signaled. That's literally what you were demonstrating with that hunk that looked wrong to me. Which suddenly doesn't look wrong anymore. Well, there was no way I was getting through this without an "I can't read" moment.
- if (async->data.apc) + if (async->data.apc && !(async->blocking && async->canceled)) { union apc_call data; memset( &data, 0, sizeof(data) ); @@ -525,7 +531,7 @@ void async_set_result( struct object *obj, unsigned int status, apc_param_t tota data.user.args[2] = 0; thread_queue_apc( NULL, async->thread, NULL, &data ); } - else if (async->data.apc_context && (async->pending || + else if (!async->data.apc && async->data.apc_context && (async->pending ||It'd probably be easier to read if you nest the "if !(blocking && canceled)" part inside the first if. That'd mess with the subsequent else, wouldn't it?
Though I do agree that's a difficult line. Maybe something like `if (async->data.apc && (!async->blocking || !async->canceled))`? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8592#note_110330
That'd mess with the subsequent else, wouldn't it?
Yes, exactly. Part of what makes this hard to read is that the "else if" has to be changed; the change I suggested would avoid that.
Though I do agree that's a difficult line. Maybe something like `if (async->data.apc && (!async->blocking || !async->canceled))`?
I dunno if that's an improvement. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8592#note_110331
participants (4)
-
Alfred Agrell (＠Alcaro) -
Elizabeth Figura (＠zfigura)
-
Paul Gofman
-
Paul Gofman (＠gofman)