From 15954c54f580951ffc9f21efcf9b623b1941803d Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Thu, 22 Dec 2022 18:57:35 +0100 Subject: [PATCH 1/3] feat(NODE-4598): close cursor on early loop break --- src/cursor/abstract_cursor.ts | 42 +++++++++++-------- .../cursor_async_iterator.test.js | 11 +++++ 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/src/cursor/abstract_cursor.ts b/src/cursor/abstract_cursor.ts index 95b8bdaef7c..a292c4d8450 100644 --- a/src/cursor/abstract_cursor.ts +++ b/src/cursor/abstract_cursor.ts @@ -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); } } } diff --git a/test/integration/node-specific/cursor_async_iterator.test.js b/test/integration/node-specific/cursor_async_iterator.test.js index 4635c414541..0ac96235460 100644 --- a/test/integration/node-specific/cursor_async_iterator.test.js +++ b/test/integration/node-specific/cursor_async_iterator.test.js @@ -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; + }); }); }); From 019fe35f61f24247ace6f45e28326413af1fa282 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Thu, 22 Dec 2022 19:37:55 +0100 Subject: [PATCH 2/3] chore: update 5.0 doc --- etc/notes/CHANGES_5.0.0.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/etc/notes/CHANGES_5.0.0.md b/etc/notes/CHANGES_5.0.0.md index e6a75108ad8..10b1d7e90c8 100644 --- a/etc/notes/CHANGES_5.0.0.md +++ b/etc/notes/CHANGES_5.0.0.md @@ -42,3 +42,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 +``` From 7e32dbe78b49465ea918bfed9ac344b36aa84401 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Thu, 22 Dec 2022 20:39:13 +0100 Subject: [PATCH 3/3] test: adding additional cases --- .../cursor_async_iterator.test.js | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/integration/node-specific/cursor_async_iterator.test.js b/test/integration/node-specific/cursor_async_iterator.test.js index 0ac96235460..f0c5254914a 100644 --- a/test/integration/node-specific/cursor_async_iterator.test.js +++ b/test/integration/node-specific/cursor_async_iterator.test.js @@ -1,6 +1,7 @@ 'use strict'; const { expect } = require('chai'); +const sinon = require('sinon'); describe('Cursor Async Iterator Tests', function () { context('default promise library', function () { @@ -36,6 +37,7 @@ describe('Cursor Async Iterator Tests', function () { } expect(counter).to.equal(1000); + expect(cursor.closed).to.be.true; }); it('should be able to use a for-await loop on an aggregation cursor', async function () { @@ -48,6 +50,7 @@ describe('Cursor Async Iterator Tests', function () { } expect(counter).to.equal(1000); + expect(cursor.closed).to.be.true; }); it('should be able to use a for-await loop on a command cursor', { @@ -64,6 +67,8 @@ describe('Cursor Async Iterator Tests', function () { } expect(counter).to.equal(indexes.length); + expect(cursor1.closed).to.be.true; + expect(cursor2.closed).to.be.true; } }); @@ -78,6 +83,7 @@ describe('Cursor Async Iterator Tests', function () { } expect(count).to.equal(1); + expect(cursor.closed).to.be.true; }); it('cleans up cursor when breaking out of for await of loops', async function () { @@ -90,5 +96,23 @@ describe('Cursor Async Iterator Tests', function () { expect(cursor.closed).to.be.true; }); + + it('returns when attempting to reuse the cursor after a break', async function () { + const cursor = collection.find(); + const spy = sinon.spy(cursor); + + for await (const doc of cursor) { + expect(doc).to.exist; + break; + } + + expect(cursor.closed).to.be.true; + + for await (const doc of cursor) { + expect.fail('Async generator returns immediately if cursor is closed', doc); + } + // cursor.close() should only be called once. + expect(spy.close.calledOnce).to.be.true; + }); }); });