fix: avoid orphaning chained and "other-queue" callbacks #46
Conversation
This fixes one of two causes of orphaned callbacks. (The other cause is that only one of the two queues gets flushed after each invocation). Previously, if a callback might have called `getProcessList` or `getProcessTree` again (which does *not* necessarily mean a recursion problem), that callback would be added to the queue, but not processed, and no `native.getProcessList` would be invoked. The callback would remain orphaned until another invocation caused that queue to be flushed again. To avoid this, we repeat the draining until the queue is empty. We use "queue.splice(0)" to atomically clear the queue, returning a batch of callbacks to process. If any of those also made requests, we repeat until the callback chain completes. An alternative would be to splice the queue once and immediately reset `requestInProgress` before invoking callbacks: `CreateToolhelp32Snapshot` has safely completed at this point. However, that would circumvent the "too many requests" rate-limiting (?) concern mentioned in one of the comments.
This removes the dependency on the per-transform/-callback result types (e.g. `IProcessTreeNode`) and "extra" arguments (e.g. `rootPid`). `getRawProcessList` just handles "max-one" gating of those requests and invocation of the callbacks with that (raw) result.
Since the transformations/filters are tracked per callback, there's no need anymore to have multiple request queues. In fact, the implementation of multiple queues would orphan callbacks in one queue if a request was active on another one, since only one set of queue's callbacks would be invoked. That no longer can happen.
rwe
force-pushed
the
fix-orphan-callbacks
branch
from
1168930 to
bca6934
Compare
rwe
changed the title
rwe
changed the title
|
Sorry @Tyriar I didn't see that you'd reviewed this already! My rebase included two new commits which together fix an additional source of stale callbacks: if The requirement to have two queues was only due to keeping that transform/filter function separate from the callback, but that was unnecessary. By binding those higher, it's possible to use a single queue and also be rid of all the extra generic-type noise. |
|
The cpp CI failure seems probably unrelated: https://github.com/microsoft/vscode-windows-process-tree/actions/runs/3432195352/jobs/5721205521 |
|
Surprisingly we both were looking at this at the same time 5 days after the initial PR 😆 I requeued CI to see if it was just a flake. |
Previously, if a callback might have called
getProcessList/getProcessTreeagain (which does not necessarily mean a recursion problem), that callback would be added to the queue, but not processed, and nonative.getProcessListwould be invoked. The callback would remain orphaned until another invocation caused that queue to be flushed again.To avoid this, we repeat the draining until the queue is empty. We use "queue.splice(0)" to atomically clear the queue, returning a batch of callbacks to process. If any of those also made requests, we repeat until the callback chain completes.
An alternative would be to splice the queue once and immediately reset
requestInProgressbefore invoking callbacks:CreateToolhelp32Snapshothas safely completed at this point. However, that would circumvent the "too many requests" rate-limiting (?) concern mentioned in one of the comments.