From eebc65a668098be6e504c0d47d728e6777b126f1 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 27 Jun 2024 06:15:49 -0400 Subject: [PATCH 1/4] fix(NODE-6242): close becomes true after calling close when documents still remain --- src/cursor/abstract_cursor.ts | 22 ++++++- .../node-specific/abstract_cursor.test.ts | 63 +++++++++++++++++++ 2 files changed, 83 insertions(+), 2 deletions(-) diff --git a/src/cursor/abstract_cursor.ts b/src/cursor/abstract_cursor.ts index cac7e8f493..623701057d 100644 --- a/src/cursor/abstract_cursor.ts +++ b/src/cursor/abstract_cursor.ts @@ -209,6 +209,17 @@ export abstract class AbstractCursor< } } + /** + * The cursor has no id until it receives a response from the initial cursor creating command. + * + * It is non-zero for as long as the database has an open cursor. + * + * The initiating command may receive a zero id if the entire result is in the `firstBatch`. + * + * The id value may still be non-zero after calling `cursor.close()` + * since the value is only updated when the server returns a new id value, + * and a cursor may be closed before iteration is complete. + */ get id(): Long | undefined { return this.cursorId ?? undefined; } @@ -249,10 +260,17 @@ export abstract class AbstractCursor< this.cursorSession = clientSession; } + /** + * The cursor is closed and all remaining locally buffered documents have been iterated. + */ get closed(): boolean { - return this.isClosed; + return this.isClosed && (this.documents?.length ?? 0) === 0; } + /** + * A `killCursors` command was attempted on this cursor. + * This is performed if the cursor id is non zero. + */ get killed(): boolean { return this.isKilled; } @@ -294,7 +312,7 @@ export abstract class AbstractCursor< return; } - if (this.closed && (this.documents?.length ?? 0) === 0) { + if (this.closed) { return; } diff --git a/test/integration/node-specific/abstract_cursor.test.ts b/test/integration/node-specific/abstract_cursor.test.ts index 65d9342676..e04cef53e6 100644 --- a/test/integration/node-specific/abstract_cursor.test.ts +++ b/test/integration/node-specific/abstract_cursor.test.ts @@ -7,6 +7,7 @@ import { inspect } from 'util'; import { type Collection, type FindCursor, + Long, MongoAPIError, type MongoClient, MongoServerError @@ -299,4 +300,66 @@ describe('class AbstractCursor', function () { expect(error).to.be.instanceof(MongoServerError); }); }); + + describe('cursor end state', function () { + let client: MongoClient; + let cursor: FindCursor; + + beforeEach(async function () { + client = this.configuration.newClient(); + const test = client.db().collection('test'); + await test.deleteMany({}); + await test.insertMany([{ a: 1 }, { a: 2 }, { a: 3 }, { a: 4 }]); + }); + + afterEach(async function () { + await cursor.close(); + await client.close(); + }); + + describe('when the last batch has been received', () => { + it('has a zero id and is not closed and is never killed', async function () { + cursor = client.db().collection('test').find({}); + expect(cursor).to.have.property('closed', false); + await cursor.tryNext(); + expect(cursor.id.isZero()).to.be.true; + expect(cursor).to.have.property('closed', false); + expect(cursor).to.have.property('killed', false); + }); + }); + + describe('when the last document has been iterated', () => { + it('has a zero id and is closed and is never killed', async function () { + cursor = client.db().collection('test').find({}); + await cursor.next(); + await cursor.next(); + await cursor.next(); + await cursor.next(); + expect(await cursor.next()).to.be.null; + expect(cursor.id.isZero()).to.be.true; + expect(cursor).to.have.property('closed', true); + expect(cursor).to.have.property('killed', false); + }); + }); + + describe('when some documents have been iterated and the cursor is closed', () => { + it('has a nonzero id and is closed and is killed', async function () { + cursor = client.db().collection('test').find({}, { batchSize: 2 }); + await cursor.next(); + await cursor.close(); + expect(cursor).to.have.property('closed', false); + expect(cursor).to.have.property('killed', true); + expect(cursor.id.isZero()).to.be.false; + + // After closing, next still returns just fine. + // I do not think this is part of our expected/intended use + // but I leave these assertions here so when/if a change is made + // it is done intentionally + expect(await cursor.next()).to.have.property('a', 2); + expect(await cursor.next()).to.be.null; + // now closed is true since we finished all the local documents + expect(cursor).to.have.property('closed', true); + }); + }); + }); }); From 5b6f75127b2f8dcddf554f7d2424c443f4e6b023 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 27 Jun 2024 10:30:28 -0400 Subject: [PATCH 2/4] fix: tests --- .../node-specific/abstract_cursor.test.ts | 13 +++++++++---- .../node-specific/cursor_async_iterator.test.js | 7 ++++--- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/test/integration/node-specific/abstract_cursor.test.ts b/test/integration/node-specific/abstract_cursor.test.ts index e04cef53e6..1237ad04f5 100644 --- a/test/integration/node-specific/abstract_cursor.test.ts +++ b/test/integration/node-specific/abstract_cursor.test.ts @@ -7,7 +7,6 @@ import { inspect } from 'util'; import { type Collection, type FindCursor, - Long, MongoAPIError, type MongoClient, MongoServerError @@ -194,7 +193,9 @@ describe('class AbstractCursor', function () { const error = await cursor.toArray().catch(e => e); expect(error).be.instanceOf(MongoAPIError); - expect(cursor.closed).to.be.true; + expect(cursor.id.isZero()).to.be.true; + // The first batch exhausted the cursor, the only thing to clean up is the session + expect(cursor.session.hasEnded).to.be.true; }); }); @@ -226,7 +227,9 @@ describe('class AbstractCursor', function () { } } catch (error) { expect(error).to.be.instanceOf(MongoAPIError); - expect(cursor.closed).to.be.true; + expect(cursor.id.isZero()).to.be.true; + // The first batch exhausted the cursor, the only thing to clean up is the session + expect(cursor.session.hasEnded).to.be.true; } }); }); @@ -260,7 +263,9 @@ describe('class AbstractCursor', function () { const error = await cursor.forEach(iterator).catch(e => e); expect(error).to.be.instanceOf(MongoAPIError); - expect(cursor.closed).to.be.true; + expect(cursor.id.isZero()).to.be.true; + // The first batch exhausted the cursor, the only thing to clean up is the session + expect(cursor.session.hasEnded).to.be.true; }); }); }); diff --git a/test/integration/node-specific/cursor_async_iterator.test.js b/test/integration/node-specific/cursor_async_iterator.test.js index 78330ff09c..1777324abc 100644 --- a/test/integration/node-specific/cursor_async_iterator.test.js +++ b/test/integration/node-specific/cursor_async_iterator.test.js @@ -95,7 +95,7 @@ describe('Cursor Async Iterator Tests', function () { } expect(count).to.equal(1); - expect(cursor.closed).to.be.true; + expect(cursor.killed).to.be.true; }); it('cleans up cursor when breaking out of for await of loops', async function () { @@ -106,7 +106,8 @@ describe('Cursor Async Iterator Tests', function () { break; } - expect(cursor.closed).to.be.true; + // The expectation is that we have "cleaned" up the cursor on the server side + expect(cursor.killed).to.be.true; }); it('returns when attempting to reuse the cursor after a break', async function () { @@ -118,7 +119,7 @@ describe('Cursor Async Iterator Tests', function () { break; } - expect(cursor.closed).to.be.true; + expect(cursor.killed).to.be.true; for await (const doc of cursor) { expect.fail('Async generator returns immediately if cursor is closed', doc); From ccf2d880b49dfdfa946115bc18aee5f720bba961 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 27 Jun 2024 10:49:00 -0400 Subject: [PATCH 3/4] fix: cs test --- test/integration/change-streams/change_stream.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/integration/change-streams/change_stream.test.ts b/test/integration/change-streams/change_stream.test.ts index 9268288782..3fa1e792ed 100644 --- a/test/integration/change-streams/change_stream.test.ts +++ b/test/integration/change-streams/change_stream.test.ts @@ -1060,7 +1060,8 @@ describe('Change Streams', function () { await changeStreamIterator.next(); await changeStreamIterator.return(); expect(changeStream.closed).to.be.true; - expect(changeStream.cursor).property('closed', true); + expect(changeStream.cursor).property('isClosed', true); + expect(changeStream.cursor).nested.property('session.hasEnded', true); } ); From 8cb34078f55cb3119e3867022b33eb4bc0c48792 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 27 Jun 2024 11:02:24 -0400 Subject: [PATCH 4/4] fix: zero out cursor id --- src/cursor/abstract_cursor.ts | 8 +++----- .../node-specific/abstract_cursor.test.ts | 16 +++++----------- 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/src/cursor/abstract_cursor.ts b/src/cursor/abstract_cursor.ts index 623701057d..da08f1a1a6 100644 --- a/src/cursor/abstract_cursor.ts +++ b/src/cursor/abstract_cursor.ts @@ -215,10 +215,6 @@ export abstract class AbstractCursor< * It is non-zero for as long as the database has an open cursor. * * The initiating command may receive a zero id if the entire result is in the `firstBatch`. - * - * The id value may still be non-zero after calling `cursor.close()` - * since the value is only updated when the server returns a new id value, - * and a cursor may be closed before iteration is complete. */ get id(): Long | undefined { return this.cursorId ?? undefined; @@ -770,9 +766,11 @@ export abstract class AbstractCursor< !session.hasEnded ) { this.isKilled = true; + const cursorId = this.cursorId; + this.cursorId = Long.ZERO; await executeOperation( this.cursorClient, - new KillCursorsOperation(this.cursorId, this.cursorNamespace, this.selectedServer, { + new KillCursorsOperation(cursorId, this.cursorNamespace, this.selectedServer, { session }) ); diff --git a/test/integration/node-specific/abstract_cursor.test.ts b/test/integration/node-specific/abstract_cursor.test.ts index 1237ad04f5..bee2333db9 100644 --- a/test/integration/node-specific/abstract_cursor.test.ts +++ b/test/integration/node-specific/abstract_cursor.test.ts @@ -9,6 +9,7 @@ import { type FindCursor, MongoAPIError, type MongoClient, + MongoCursorExhaustedError, MongoServerError } from '../../mongodb'; @@ -348,22 +349,15 @@ describe('class AbstractCursor', function () { }); describe('when some documents have been iterated and the cursor is closed', () => { - it('has a nonzero id and is closed and is killed', async function () { + it('has a zero id and is not closed and is killed', async function () { cursor = client.db().collection('test').find({}, { batchSize: 2 }); await cursor.next(); await cursor.close(); expect(cursor).to.have.property('closed', false); expect(cursor).to.have.property('killed', true); - expect(cursor.id.isZero()).to.be.false; - - // After closing, next still returns just fine. - // I do not think this is part of our expected/intended use - // but I leave these assertions here so when/if a change is made - // it is done intentionally - expect(await cursor.next()).to.have.property('a', 2); - expect(await cursor.next()).to.be.null; - // now closed is true since we finished all the local documents - expect(cursor).to.have.property('closed', true); + expect(cursor.id.isZero()).to.be.true; + const error = await cursor.next().catch(error => error); + expect(error).to.be.instanceOf(MongoCursorExhaustedError); }); }); });