Besides, I think not caring when a thread is actually waiting on IOCP wait object or not is a great deal of simplication here, leaving all the logic in generic wineserver wait queue mechanics. This way it is done now it doesn't have to hook various wait related events and know whether thread is waiting, or timed out, or got suspended before or while waiting, all that is handled in a generic wait logic. The logic is:
Well, I was thinking you only need to remove it in two cases: when returning a packet, and when timing out. But I was also forgetting that satisfied() isn't called in the timeout case, and anyway it's a moot point.
Why do we keep destroying and recreating the completion_wait? I'd expect us to either destroy it immediately when it's not needed (cf. the above question), or keep it around for the lifetime of the thread and simply retarget it to a different IOCP.
Well, I thought it is a bit simplier this way. Completion wait is associated with (thread, IOCP) pair since creation until destroy, there is no transition, just creation and universal cleanup. But also it is not hard to retarget to a different IOCP in (remove_completion), I will do it this way.
Eh, the question was half motivated by not quite understanding the removal logic ye·t.
The one advantage of doing it this way is that we get rid of a server object when it's not needed. I don't know if that's worthwhile or not.
It seems to me 4/6 fixes a different bug? There are two of them (not exactly similar):
- A thread is waiting on completion port, either directly with WaitFor... from the app or from NtRemoveIoCompletion (now on separate object). The last handle to the port is closed (without any completion available on port). Currently the waits will hang forever (or until timeout, if any). This is fixed by 4/6.
- A thread is waiting in NtRemoveIoCompletionEx. Another thread posts completion and closes handle. Both currently and with these patches the waiting thread will be woken by the posted completion. But currently attempting to get the completion will fail because port handle is already invalid. This is solved by 2/6.
Right, that's what I was broadly understanding, and that's what the commit messages communicate too.
But as of 2/6 the completion_wait is still destroyed when a completion is destroyed, and the associated packet with it. This is fixed by the "if (!wait->msg)" before calling cleanup_thread_completion(), but that's not done until 4/6. Or maybe I'm missing something here?
Why the "ok(count <= 1)"? Does this differ between Windows versions, and, if so, can we document this in the code please?
Yes, it is older Windows behaviour (probably a small bug?) where NtRemoveIoCompletionEx() will always return 1 in the output completion count in case of errors (while actually no completion is returned). It is not the case anymore at least on Win11 and the same test_set_io_completion() has the same below where I didn't touch anything. I guess in this place I will just remove this check, it is irrelevant here.
Ah, I should have checked if it was copypasted, sorry about that.