This works but it looks fragile. I haven't actually checked, but is there any other way that the same async can be terminated twice?
Yes. Consider the following scenario on `async`:
1. I/O timeout is configured for `async` via an `async_set_timeout()` call. 1. `async` enters alerted state. 2. An application calls `NtCancelIoFileEx` with `async`'s user IOSB. 3. `async_terminate( async, STATUS_CANCELLED )` is called. 4. The timeout expires. 5. `async_terminate( async, async->timeout_status );` is called. (`async->timeout_status` is usually `STATUS_TIMEOUT`.)
Furthermore, I/O cancellation and timeout can race with other async termination requests.
If so, is it correct to buffer the second termination (and not, say, buffer a third?)
I don't think so. `async_terminate()` is currently no-op if the async has already been terminated with a status other than `STATUS_ALERTED`, and the patch maintains this invariant.
If not, should we make this explicitly a boolean flag ('canceled' I guess?)
It is possible to introducing a new boolean flag, but the flag cannot replace `terminate_status`. `terminate_status` can be any arbitrary value passed as the third parameter of `async_set_timeout`.
Alternatively, we can rename `terminate_status` to `cancel_status` or `cancellation_status` (may be a bit misleading: is a timeout a cancellation?)
Or at least add some asserts to communicate exactly when this is supposed to happen?
Right now I don't see any additional invariants that the new code paths would expect. `terminate_status` simply means the first status that the async was terminated with, other than `STATUS_ALERTED`. Termination can happen regardless of whether the async is in alerted state, but `terminate_status` would be used in `async_set_result` only if `async->alerted`. Can you suggest what the assertions would be?
-
Jinoh Kang (＠iamahuman)