From 5e0491f2ec5e194f4499aef08e1bb1d4f6db6611 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 12 Jul 2021 17:16:12 +0200 Subject: [PATCH 1/2] fix(service-provider-server): fix change stream .next() interrupt MONGOSH-888 When a user interrupts running code while a change stream `.next()` call is running, that closes the network connection. However, network errors are considered retryable by the driver (in line with the spec); this makes the change stream cursor `.next()` operation attempt to re-try itself, which fails because the MongoClient is already unusable at that point. In order to address this, we call the driver-internal `Connection.handleIssue()` method (which should be okay, since the relevant code here is concerned only with driver internals anyway) with a non-network-error state, which still closes ongoing operations. --- .../service-provider-server/src/mongodb-patches.ts | 13 ++++++++++++- packages/shell-api/src/change-stream-cursor.spec.ts | 13 +++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/packages/service-provider-server/src/mongodb-patches.ts b/packages/service-provider-server/src/mongodb-patches.ts index c2ad5580a1..b767186a01 100644 --- a/packages/service-provider-server/src/mongodb-patches.ts +++ b/packages/service-provider-server/src/mongodb-patches.ts @@ -66,7 +66,18 @@ function patchConnectionPoolTracking(): void { const originalCallback = cb; cb = function(this: any, error: any, result: any) { poolToConnections.delete(pool); - [...connections].forEach(c => c.destroy({ force: true })); + for (const c of connections) { + c.destroy({ force: true }); + // Immediately after destroying, act as if the close had happened, + // but *not* as an actual 'close' event on the socket itself -- + // a close on the socket is communicated as a network error, which + // is considered an retryable error by operations which are currently + // running on this connection, but the whole point here is that these + // operations should *not* be retried. So, we just act as if something + // had happened that interrupts all ongoing operations and also is + // supposed to destroy the connection (which is a no-op at this point). + c.handleIssue({ destroy: true }); + } if (originalCallback) { originalCallback.call(this, error, result); diff --git a/packages/shell-api/src/change-stream-cursor.spec.ts b/packages/shell-api/src/change-stream-cursor.spec.ts index 0894feb11a..2b12b3b280 100644 --- a/packages/shell-api/src/change-stream-cursor.spec.ts +++ b/packages/shell-api/src/change-stream-cursor.spec.ts @@ -230,6 +230,19 @@ describe('ChangeStreamCursor', () => { 'itcount to return 1'); expect(result).to.equal(1); }); + it('can be interrupted when .next() blocks', async() => { + const nextPromise = cursor.next(); + nextPromise.catch(() => {}); // Suppress UnhandledPromiseRejectionWarning + await new Promise(resolve => setTimeout(resolve, 100)); + expect(await internalState.onInterruptExecution()).to.equal(true); + expect(await internalState.onResumeExecution()).to.equal(true); + try { + await nextPromise; + expect.fail('missed exception'); + } catch (err) { + expect(err.name).to.equal('MongoshInterruptedError'); + } + }); }); describe('mongo watch', () => { beforeEach(async() => { From dc1300e395f825d542b7b778c334cbb5771939df Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 13 Jul 2021 12:33:03 +0200 Subject: [PATCH 2/2] fixup: try destroying with error --- packages/service-provider-server/src/mongodb-patches.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/service-provider-server/src/mongodb-patches.ts b/packages/service-provider-server/src/mongodb-patches.ts index b767186a01..80c84c6c81 100644 --- a/packages/service-provider-server/src/mongodb-patches.ts +++ b/packages/service-provider-server/src/mongodb-patches.ts @@ -76,7 +76,7 @@ function patchConnectionPoolTracking(): void { // operations should *not* be retried. So, we just act as if something // had happened that interrupts all ongoing operations and also is // supposed to destroy the connection (which is a no-op at this point). - c.handleIssue({ destroy: true }); + c.handleIssue({ destroy: new Error('connection canceled by force close') }); } if (originalCallback) {