AsyncIterator icon indicating copy to clipboard operation
AsyncIterator copied to clipboard

Intermediate async iterator does not correctly emit readable event, unpredictably

Open gsvarovsky opened this issue 3 years ago • 2 comments

I'm getting a behaviour that appears to be caused by a residual task scheduler ordering issue, related to #28.

I have a number of unit tests that involve async iterators returned from node-quadstore. In some cases the test can hang and timeout. The hangs are consistently reproducible but unpredictable. If some tests are skipped, the hang happens in a different place; but the tests are fully independent with respect to everything but globals.

Debugging shows that the hang is caused by an async iterator which is not emitting a readable event despite having new items available.

The screenshot shows a debug session in which the readable event is about to be suppressed because the async iterator already has this._readable === true.

I have two reasons to suspect a task ordering issue:

  1. This code is called in the task which was scheduled in handling the new readable event from the source LevelIterator (you can see that the source is readable and that the destination SimpleTransformIterator is not yet readable).
  2. Calling setTaskScheduler(queueMicrotask) early in my setup fixes the issue.

image

I will continue to investigate. I may try to create a test scenario, but it may be more fruitful for me to offer a hypothesis and a fix instead. Notifying you here in case you have any ideas! 🙂

gsvarovsky avatar Nov 19 '22 21:11 gsvarovsky

Hi @gsvarovsky thanks for the report! Just so you are aware; I'm working on a rewrite of this library, and that rewrite is more determinate about how the end, and readable events are emitted.

I cannot guarantee it will be released any time soon as it may take some time to properly review. But in the same token I would also not spend a significant amount of time on test scenarios that are specific to the current design of the code.

jeswr avatar Nov 20 '22 01:11 jeswr

Thanks @jeswr

Since this will be addressed by your rewrite, but in the meantime can cause hangs in tests and (theoretically) apps, I'm going to release a patch of m-ld that exclusively uses queueMicrotask.

gsvarovsky avatar Nov 21 '22 12:11 gsvarovsky