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."