QueryCursor asyncIterator may not work if the iteration body is asynchronous

[dbellavista]

Do you want to request a feature or report a bug?

bug

What is the current behavior?

Sample script: https://gist.github.com/dbellavista/f9523dc900a3e93a02d4027295897499

the QueryCursor inherit from stream.Readable the [Symbol.asyncIterable] method (see https://nodejs.org/api/stream.html#stream_readable_symbol_asynciterator), allowing it to be used inside the for await or as an async iterable.

// For await
for await (const el of cursor) {}
// Low level iterator
const iterable = cursor[Symbol.asyncIterator]();

Which basically invokes continuously the _read method until the internal buffer is filled. The problem is that when the body of the for await is asyncronous. The _read method closes the cursor and emits the close event

mongoose/lib/cursor/QueryCursor.js

Lines 90 to 96 in 1729a8b

	setTimeout(function() {
	// on node >= 14 streams close automatically (gh-8834)
	const isNotClosedAutomatically = !_this.destroyed;
	if (isNotClosedAutomatically) {
	_this.emit('close');
	}
	}, 0);

I think the Readable stream doesn't like this behaviour and stops the async iteration. By increasing the cursor setTimeout from 0 to 1000 the sample script executes correctly.

What happen is:

1. iterable.next()
2. Readable._read is invoked and the next value is served to the iteration body
3. The iteration body executes but goes async
4. The Readable._read is invoked in "background" until the buffer is full
5. The Cursor has no more elements, it's closed and the emit event is forcefully invoked
6. The iteration body continue to execute and invokes iterable.next() again
7. The Readable stream has been closed so the iteration is over

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.

• node 12.18.3
• mongoose 5.10.3
• mongodb 4.2.9

[vkarpov15]

I believe this is almost a duplicate of #8280, although #8280 is for aggregation cursors rather than query cursors.

TLDR; the solution is to not use async iterators with Model.find().cursor(). You should use async iterators with Model.find() instead:

// Don't do this! Uses async iterator for streams under the hood
const cursor = Model.find().cursor();
for await (const v of cursor) {
  await wait();
  countForAwaitAsync += 1;
}
await cursor.close();

// Do this instead
for await (const v of Model.find()) {
  await wait();
  countForAwaitAsync += 1;
}

As you mentioned, the problem comes down to the fact that, for legacy reasons, QueryCursor is a Node.js stream, and so it inherits streams' built-in async iterator capability. Unfortunately, streams' built-in async iterator has some quirks with pull mode vs push mode.

We should make QueryCursors support async iteration in a future release to avoid this potential gotcha, or at least throw an error if the user tries to use a cursor as a stream

[dbellavista]

I understand and I agree the best curse of action should be to throw and exception when trying to access to the asynIterator method, otherwise it may be difficult to spot the error