Skip to content

feat(NODE-4598)!: close cursor on early loop break #3502

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Dec 22, 2022
Merged

feat(NODE-4598)!: close cursor on early loop break #3502

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
15954c5
feat(NODE-4598): close cursor on early loop break
durran Dec 22, 2022
019fe35
chore: update 5.0 doc
durran Dec 22, 2022
f368829
Merge branch 'main' into NODE-4598
durran Dec 22, 2022
d1dafd8
Merge branch 'main' into NODE-4598
durran Dec 22, 2022
7e32dbe
test: adding additional cases
durran Dec 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions etc/notes/CHANGES_5.0.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,17 @@ The new minimum supported Node.js version is now 14.20.1.

The MongoClient option `promiseLibrary` along with the `Promise.set` export that allows specifying a custom promise library has been removed.
This allows the driver to adopt async/await syntax which has [performance benefits](https://v8.dev/blog/fast-async) over manual promise construction.

### Cursor closes on exit of for await of loops

Cursors will now automatically close when exiting a for await of loop on the cursor itself.

```js
const cursor = collection.find({});
for await (const doc of cursor) {
console.log(doc);
break;
}

cursor.closed // true
```
42 changes: 25 additions & 17 deletions src/cursor/abstract_cursor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,29 +301,37 @@ export abstract class AbstractCursor<
return;
}

while (true) {
const document = await this.next();
try {
while (true) {
const document = await this.next();

// Intentional strict null check, because users can map cursors to falsey values.
// We allow mapping to all values except for null.
// eslint-disable-next-line no-restricted-syntax
if (document === null) {
if (!this.closed) {
const message =
'Cursor returned a `null` document, but the cursor is not exhausted. Mapping documents to `null` is not supported in the cursor transform.';
// Intentional strict null check, because users can map cursors to falsey values.
// We allow mapping to all values except for null.
// eslint-disable-next-line no-restricted-syntax
if (document === null) {
if (!this.closed) {
const message =
'Cursor returned a `null` document, but the cursor is not exhausted. Mapping documents to `null` is not supported in the cursor transform.';

await cleanupCursorAsync(this, { needsToEmitClosed: true }).catch(() => null);
await cleanupCursorAsync(this, { needsToEmitClosed: true }).catch(() => null);

throw new MongoAPIError(message);
throw new MongoAPIError(message);
}
break;
}
break;
}

yield document;
yield document;

if (this[kId] === Long.ZERO) {
// Cursor exhausted
break;
if (this[kId] === Long.ZERO) {
// Cursor exhausted
break;
}
}
} finally {
// Only close the cursor if it has not already been closed. This finally clause handles
// the case when a user would break out of a for await of loop early.
if (!this.closed) {
await this.close().catch(() => null);
}
}
}
Expand Down
11 changes: 11 additions & 0 deletions test/integration/node-specific/cursor_async_iterator.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,5 +79,16 @@ describe('Cursor Async Iterator Tests', function () {

expect(count).to.equal(1);
});

it('cleans up cursor when breaking out of for await of loops', async function () {
const cursor = collection.find();

for await (const doc of cursor) {
expect(doc).to.exist;
break;
}

expect(cursor.closed).to.be.true;
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add just a couple more cases / Qs:

  • Won't the finally logic always be run at the end of the loop or is it only when there's break? We can add an assertion to the existing tests for the expected "closed" state either way.
  • Can we add a test that attempt to keep using the cursor after the for await has done the clean up, ex. expect(async () => for await (xx)).to.throw(X)
  • We have a gate on calling close only if it has not been called already (using the closed flag), I'm not sure it's easily reachable since we have the early return based on closed=true. I think it's worth capturing that behavior's significance, does close fail or repeat steps if it's called more than once?
    • We could use the iterator manually, call cursor[Symbol.asyncIterator]() and use .return() + sinon to check that .close is only called once
    • We could double check that close does not repeat steps if called more than once (this may be more trouble than it's worth in this PR, but I think needsToEmitClosed based on !this[kClosed] provides idempotency).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added assertions to the other tests that checks the cursor is closed on every one once they are exahusted. I've also added a test that asserts the second attempted iteration immediately returns, and also spies on the cursor to assert that close() is called only once. I think these cover the points you were getting at here but let me know if I missed something.

});
});