Re: [PATCH v3 0/1] MR499: server: Allow cancelling alerted asyncs.

[Jinoh Kang (＠iamahuman)]

On Tue Apr 18 19:25:54 2023 +0000, Zebediah Figura wrote:
> > 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.
> > Right, that's the bit I failed to think through.
> > It is possible to introduce a new boolean flag, but the flag cannot
> > replace `terminate_status`. `terminate_status` can be any arbitrary
> > value passed to the third parameter of `async_set_timeout`.
> > Ah, and I missed that that was a possibility, although, in my defense,
> > it's not stated in the patch subject either. I.e. this patch doesn't
> > just fix the race where an alerted async is canceled, it also fixes the
> > similar race where it times out.
> > So yes, I think the patch makes sense as is. Maybe a comment to yet
> > again overdocument asyncs wouldn't hurt? I easily see myself looking at
> > code like this and having to ask myself, "when exactly is
> > terminate_status set?" and the answer is "if (and only if?) the async is
> > canceled or times out while it's alerted", which is nice to have
> > documented and not have to puzzle through later.
> > To reduce complexity, I've tried refactoring the state management.  Currently, the async has the following two state flags that deal with the state of the client that originally requested the async:

- `signaled`: indicates whether the async object is signaled; the client may wait on the async handle before returning from the corresponding wine system call.
  - Valid transition: 0 -> 1.
  - This flag is set if only if any of the following conditions are true:
    - Given that `!async->blocking`,
      - `async_wake_obj(async)` has been called when `async->unknown_status = 0`, or
      - `async_handoff(async, ...)` has been called with `get_error() != STATUS_ALERTED && (async->pending || !NT_ERROR(get_error()) && (get_error() != STATUS_PENDING || async->iosb->status != STATUS_PENDING)`.
    - (Regardless of `async->blocking`) `async_set_result(async, status, ...)` has been called with `async->terminated && (!async->alerted || status != STATUS_PENDING)`.
  - Note that this flag exhibits slightly different behavior depending on `async->blocking`.
    - If `async->blocking`, it tries to block the requestor until the async operation has completed.
    - If `!async->blocking`, it tries to signal as soon as the initial status is known, _unless_ the wait handle is being closed, in which case it doesn't bother to touch the flag at all.
  - If signaled and a wait on the async object is satisfied, the requestor function should have performed the synchronous completion sequence (either directly or via APC\_ASYNC\_IO) before the wait is satisfied, unless `async->pending`.
- `direct_result`: from Wine codebase, `a flag if we're passing result directly from request instead of APC`.
  - Valid transition: 1 -> 0 (after set to 1 from `create_request_async`).
  - Whenever async state is settled, the flag is set if all of the following conditions are true:
     - The async was created via create_request_async(), and
     - async_set_unknown_status() has not been called, and
     - The async handle has never been waited on (wait consumes the flag), and
     - Any of:
       - async_handoff() has not been called, or
       - From async_handoff(), the async is terminated or alerted synchronously.
  - The flag has the following effect:
    - For each async_terminate() call, it will send APC_ASYNC_IO if and only if !direct_result.
    - For each async handle wait satisfaction, it will call async_set_result() if and only if direct_result.

We can observe the following from above:

- The flags are defined in terms of what the flags *do* rather than what the value of the flags *mean*.  It doesn't help that the transitions for each flag are quite scattered and sparse.
- Their behaviors are dependent on the client-side async implementation, so reasoning about them requires verifying how the requestor side behaves.
- Depending on other part of the async object state, one or more flags may be left unobserved and thus ignored, effectivly becoming a "don't care" bit.

This increases complexity and hinders maintenance in my opinion.  In particular, it's difficult to determine precisely *when* the flag is or should be set or unset.

I'd propose that the constituent conditions of each flag be refactored into separate state variables first.  Paradoxically, it may even help combine two or more binary flags into an enum as the semantics become more simpler.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/499#note_31579