From 9312963b3d9e000ac619ee3d93a57b938da27a0b Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Wed, 29 Jan 2020 19:24:28 +0100 Subject: [PATCH 1/2] fix: retry 'Session not found' errors on getSnapshot If the BeginTransaction RPC of a read-only transaction fails with a 'Session not found' error, we can safely discard the invalid session, get a new session from the pool and retry the BeginTransaction call on the new session. --- src/database.ts | 10 +++++-- test/database.ts | 43 ++++++++++++++++++++++++-- test/mockserver/mockspanner.ts | 55 ++++++++++++++++++++-------------- test/spanner.ts | 54 +++++++++++++++++++++++++++++++++ 4 files changed, 134 insertions(+), 28 deletions(-) diff --git a/src/database.ts b/src/database.ts index 0fe6245c4..8f94b3485 100644 --- a/src/database.ts +++ b/src/database.ts @@ -1223,8 +1223,14 @@ class Database extends GrpcServiceObject { snapshot.begin(err => { if (err) { - this.pool_.release(session!); - callback!(err); + if (isSessionNotFoundError(err)) { + session!.lastError = err; + this.pool_.release(session!); + this.getSnapshot(options, callback!); + } else { + this.pool_.release(session!); + callback!(err); + } return; } diff --git a/test/database.ts b/test/database.ts index 3cbc57cd0..f0831483a 100644 --- a/test/database.ts +++ b/test/database.ts @@ -18,18 +18,17 @@ import * as assert from 'assert'; import {describe, it} from 'mocha'; import {EventEmitter} from 'events'; import * as extend from 'extend'; -import {ApiError} from '@google-cloud/common'; +import {ApiError, util} from '@google-cloud/common'; import * as proxyquire from 'proxyquire'; import * as sinon from 'sinon'; import {Transform} from 'stream'; import * as through from 'through2'; -import {util} from '@google-cloud/common'; import * as pfy from '@google-cloud/promisify'; import * as db from '../src/database'; import {Instance} from '../src'; import {TimestampBounds} from '../src/transaction'; import {ServiceError, status} from 'grpc'; -import {isSessionNotFoundError} from '../src/session-pool'; +import {MockError} from './mockserver/mockspanner'; let promisified = false; const fakePfy = extend({}, pfy, { @@ -1734,6 +1733,44 @@ describe('Database', () => { }); }); + it('should retry if `begin` errors with `Session not found`', done => { + const fakeError = { + code: status.NOT_FOUND, + message: 'Session not found', + } as MockError; + + const fakeSession2 = new FakeSession(); + const fakeSnapshot2 = new FakeTransaction(); + sandbox + .stub(fakeSnapshot2, 'begin') + .callsFake(callback => callback(null)); + sandbox.stub(fakeSession2, 'snapshot').returns(fakeSnapshot2); + + getReadSessionStub + .onFirstCall() + .callsFake(callback => callback(null, fakeSession)) + .onSecondCall() + .callsFake(callback => callback(null, fakeSession2)); + beginSnapshotStub.callsFake(callback => callback(fakeError)); + + // The first session that was not found should be released back into the + // pool, so that the pool can remove it from its inventory. + const releaseStub = sandbox.stub(fakePool, 'release'); + + database.getSnapshot((err, snapshot) => { + assert.ifError(err); + assert.strictEqual(snapshot, fakeSnapshot2); + // The first session that error should already have been released back + // to the pool. + assert.strictEqual(releaseStub.callCount, 1); + // Ending the valid snapshot will release its session back into the + // pool. + snapshot.emit('end'); + assert.strictEqual(releaseStub.callCount, 2); + done(); + }); + }); + it('should return the `snapshot`', done => { database.getSnapshot((err, snapshot) => { assert.ifError(err); diff --git a/test/mockserver/mockspanner.ts b/test/mockserver/mockspanner.ts index 7d1b58c23..926104baa 100644 --- a/test/mockserver/mockspanner.ts +++ b/test/mockserver/mockspanner.ts @@ -556,30 +556,39 @@ export class MockSpanner { call: grpc.ServerUnaryCall, callback: protobuf.Spanner.BeginTransactionCallback ) { - const session = this.sessions.get(call.request.session); - if (session) { - let counter = this.transactionCounters.get(session.name); - if (!counter) { - counter = 0; - } - const id = ++counter; - this.transactionCounters.set(session.name, counter); - const transactionId = id.toString().padStart(12, '0'); - const fullTransactionId = session.name + '/transactions/' + transactionId; - const readTimestamp = - call.request.options && call.request.options.readOnly - ? now() - : undefined; - const transaction = protobuf.Transaction.create({ - id: Buffer.from(transactionId), - readTimestamp, + this.simulateExecutionTime(this.beginTransaction.name) + .then(() => { + const session = this.sessions.get(call.request.session); + if (session) { + let counter = this.transactionCounters.get(session.name); + if (!counter) { + counter = 0; + } + const id = ++counter; + this.transactionCounters.set(session.name, counter); + const transactionId = id.toString().padStart(12, '0'); + const fullTransactionId = + session.name + '/transactions/' + transactionId; + const readTimestamp = + call.request.options && call.request.options.readOnly + ? now() + : undefined; + const transaction = protobuf.Transaction.create({ + id: Buffer.from(transactionId), + readTimestamp, + }); + this.transactions.set(fullTransactionId, transaction); + this.transactionOptions.set(fullTransactionId, call.request.options); + callback(null, transaction); + } else { + callback( + MockSpanner.createSessionNotFoundError(call.request.session) + ); + } + }) + .catch(err => { + callback(err); }); - this.transactions.set(fullTransactionId, transaction); - this.transactionOptions.set(fullTransactionId, call.request.options); - callback(null, transaction); - } else { - callback(MockSpanner.createSessionNotFoundError(call.request.session)); - } } commit( diff --git a/test/spanner.ts b/test/spanner.ts index 1f78fa81b..c83e9ca9f 100644 --- a/test/spanner.ts +++ b/test/spanner.ts @@ -615,6 +615,60 @@ describe('Spanner with mock server', () => { done(); }); }); + + it('should retry "Session not found" errors for Database.getSnapshot() with callbacks', done => { + const db = newTestDatabase(); + const sessionNotFound = { + code: status.NOT_FOUND, + message: 'Session not found', + } as MockError; + // The beginTransaction call will fail 3 times with 'Session not found' + // before succeeding. + spannerMock.setExecutionTime( + spannerMock.beginTransaction, + SimulatedExecutionTime.ofErrors([ + sessionNotFound, + sessionNotFound, + sessionNotFound, + ]) + ); + db.getSnapshot((err, snapshot) => { + assert.ifError(err); + snapshot!.run(selectSql, (err, rows) => { + assert.ifError(err); + assert.strictEqual(rows.length, 3); + snapshot!.end(); + db.close(done); + }); + }); + }); + + it('should retry "Session not found" errors for Database.getSnapshot()', done => { + const db = newTestDatabase(); + spannerMock.setExecutionTime( + spannerMock.beginTransaction, + SimulatedExecutionTime.ofError({ + code: status.NOT_FOUND, + message: 'Session not found', + } as MockError) + ); + db.getSnapshot() + .then(response => { + const [snapshot] = response; + snapshot + .run(selectSql) + .then(response => { + const [rows] = response; + assert.strictEqual(rows.length, 3); + snapshot.end(); + db.close() + .then(() => done()) + .catch(done); + }) + .catch(done); + }) + .catch(done); + }); }); describe('session-pool', () => { From 1528f6a79fdd39cc5f849b1b2e1f6aa5538497b5 Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Thu, 6 Feb 2020 07:15:50 +0100 Subject: [PATCH 2/2] fix: end test with db.close(done) --- test/spanner.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/spanner.ts b/test/spanner.ts index c83e9ca9f..d21723b64 100644 --- a/test/spanner.ts +++ b/test/spanner.ts @@ -661,9 +661,7 @@ describe('Spanner with mock server', () => { const [rows] = response; assert.strictEqual(rows.length, 3); snapshot.end(); - db.close() - .then(() => done()) - .catch(done); + db.close(done); }) .catch(done); })