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.
There is also at least one more case, suspended thread handling. If a thread gets suspended while in NtRemoveIoCompletion it is essentially not a in a wait queue, other threads can get the completion. This works correctly with generic server wait queues logic.
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.
But we will remove the object once completion port is closed, or thread is terminated. Until then, it is generally needed... and removing it will trade that object existence for the need of recreation, and also seems considerably more complicated as we need to know when the client actually got off the wait?
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?
Anyway, in v2 I reshuffled things a bit and introducing the possibility of 'detached' completion wait (with wait->completion == NULL) only in 4/6. This way it surely doesn't fix any of those two issues (both ends up fixed in 4/6) and is mostly an intermediate step. The the patch message says "ntdll: Assign completion to thread when wait for completion is satisfied.", it doesn't promise any fixes related to handle close? The functional effect is that it introduces LIFO wake up and denies stealing completions by "newcomer" threads when the wait for this completion is already satisfied.