Skip to content
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.

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

feat(NODE-6060): set fire-and-forget protocol when writeConcern is w: 0 #4219

Merged
merged 20 commits into from
Sep 17, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
6 changes: 3 additions & 3 deletions src/cmap/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ export interface OpQueryOptions extends CommandOptions {
secondaryOk?: boolean;

requestId?: number;
moreToCome?: boolean;
exhaustAllowed?: boolean;
}

Expand All @@ -74,6 +73,8 @@ export class OpQueryRequest {
awaitData: boolean;
exhaust: boolean;
partial: boolean;
/** moreToCome is an OP_MSG only concept */
moreToCome = false;

constructor(
public databaseName: string,
Expand Down Expand Up @@ -412,7 +413,6 @@ export interface OpMsgOptions {
ignoreUndefined: boolean;
checkKeys: boolean;
maxBsonSize: number;
moreToCome: boolean;
exhaustAllowed: boolean;
readPreference: ReadPreference;
}
Expand Down Expand Up @@ -465,7 +465,7 @@ export class OpMsgRequest {

// flags
this.checksumPresent = false;
this.moreToCome = options.moreToCome || false;
this.moreToCome = command.writeConcern?.w === 0 || false;
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
this.exhaustAllowed =
typeof options.exhaustAllowed === 'boolean' ? options.exhaustAllowed : false;
}
Expand Down
8 changes: 6 additions & 2 deletions src/cmap/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,7 @@ export class Connection extends TypedEventEmitter<ConnectionEvents> {
zlibCompressionLevel: this.description.zlibCompressionLevel
});

if (options.noResponse) {
if (options.noResponse || message.moreToCome) {
yield MongoDBResponse.empty;
return;
}
Expand Down Expand Up @@ -527,7 +527,11 @@ export class Connection extends TypedEventEmitter<ConnectionEvents> {
new CommandSucceededEvent(
this,
message,
options.noResponse ? undefined : (object ??= document.toObject(bsonOptions)),
options.noResponse
? undefined
: message.moreToCome
? { ok: 1 }
aditi-khare-mongoDB marked this conversation as resolved.
Show resolved Hide resolved
: (object ??= document.toObject(bsonOptions)),
started,
this.description.serverConnectionId
)
Expand Down
1 change: 1 addition & 0 deletions src/operations/command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ export interface CommandOperationOptions
// Admin command overrides.
dbName?: string;
authdb?: string;
/** @deprecated Will be removed in the next major version. Set writeConcern.w to 0 instead. */
noResponse?: boolean;
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down
1 change: 1 addition & 0 deletions src/operations/run_command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ export class RunAdminCommandOperation<T = Document> extends AbstractOperation<T>
constructor(
public command: Document,
public override options: RunCommandOptions & {
/** @deprecated Will be removed in the next major version, and replaced with an option to set writeConcern.w to 0 */
noResponse?: boolean;
aditi-khare-mongoDB marked this conversation as resolved.
Show resolved Hide resolved
bypassPinningCheck?: boolean;
}
Expand Down
5 changes: 4 additions & 1 deletion src/write_concern.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,10 @@ interface CommandWriteConcernOptions {
* @see https://www.mongodb.com/docs/manual/reference/write-concern/
*/
export class WriteConcern {
/** Request acknowledgment that the write operation has propagated to a specified number of mongod instances or to mongod instances with specified tags. */
/**
* Request acknowledgment that the write operation has propagated to a specified number of mongod instances or to mongod instances with specified tags.
* If w is 0 and is set on a write operation, the server will not send a response.
*/
readonly w?: W;
/** Request acknowledgment that the write operation has been written to the on-disk journal */
readonly journal?: boolean;
Expand Down
2 changes: 1 addition & 1 deletion test/integration/node-specific/mongo_client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -698,7 +698,7 @@ describe('class MongoClient', function () {
expect(startedEvents).to.have.lengthOf(1);
expect(startedEvents[0]).to.have.property('commandName', 'endSessions');
expect(endEvents).to.have.lengthOf(1);
expect(endEvents[0]).to.have.property('reply', undefined); // noReponse: true
expect(endEvents[0]).to.property('reply', undefined);
});

context('when server selection would return no servers', () => {
Expand Down
140 changes: 138 additions & 2 deletions test/integration/read-write-concern/write_concern.test.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
import { expect } from 'chai';
import { on, once } from 'events';
import { gte } from 'semver';
import * as sinon from 'sinon';

import {
type Collection,
type CommandStartedEvent,
type CommandSucceededEvent,
type Db,
LEGACY_HELLO_COMMAND,
MongoClient
MongoClient,
OpMsgRequest
} from '../../mongodb';
import * as mock from '../../tools/mongodb-mock/index';
import { filterForCommands } from '../shared';
Expand Down Expand Up @@ -93,7 +97,7 @@ describe('Write Concern', function () {
});

afterEach(async function () {
await db.dropDatabase();
await db.dropDatabase({ writeConcern: { w: 'majority' } });
await client.close();
durran marked this conversation as resolved.
Show resolved Hide resolved
});

Expand Down Expand Up @@ -168,4 +172,136 @@ describe('Write Concern', function () {
});
});
});

describe('fire-and-forget protocol', function () {
context('when writeConcern = 0 and OP_MSG is used', function () {
const writeOperations: { name: string; command: any; expectedReturnVal: any }[] = [
{
name: 'insertOne',
command: client => client.db('test').collection('test').insertOne({ a: 1 }),
expectedReturnVal: { acknowledged: false }
durran marked this conversation as resolved.
Show resolved Hide resolved
},
{
name: 'insertMany',
command: client =>
client
.db('test')
.collection('test')
.insertMany([{ a: 1 }, { b: 2 }]),
expectedReturnVal: { acknowledged: false }
},
{
name: 'updateOne',
command: client =>
client
.db('test')
.collection('test')
.updateOne({ i: 128 }, { $set: { c: 2 } }),
expectedReturnVal: { acknowledged: false }
},
{
name: 'updateMany',
command: client =>
client
.db('test')
.collection('test')
.updateMany({ name: 'foobar' }, { $set: { name: 'fizzbuzz' } }),
expectedReturnVal: { acknowledged: false }
},
{
name: 'deleteOne',
command: client => client.db('test').collection('test').deleteOne({ a: 1 }),
expectedReturnVal: { acknowledged: false }
},
{
name: 'deleteMany',
command: client => client.db('test').collection('test').deleteMany({ name: 'foobar' }),
expectedReturnVal: { acknowledged: false }
},
{
name: 'replaceOne',
command: client => client.db('test').collection('test').replaceOne({ a: 1 }, { b: 2 }),
expectedReturnVal: { acknowledged: false }
},
{
name: 'removeUser',
command: client => client.db('test').removeUser('albert'),
expectedReturnVal: true
},
{
name: 'findAndModify',
command: client =>
client
.db('test')
.collection('test')
.findOneAndUpdate({}, { $setOnInsert: { a: 1 } }, { upsert: true }),
expectedReturnVal: null
},
{
name: 'dropDatabase',
command: client => client.db('test').dropDatabase(),
expectedReturnVal: true
},
{
name: 'dropCollection',
command: client => client.db('test').dropCollection('test'),
expectedReturnVal: true
},
{
name: 'dropIndexes',
command: client => client.db('test').collection('test').dropIndex('a'),
expectedReturnVal: { ok: 1 }
},
{
name: 'createIndexes',
command: client => client.db('test').collection('test').createIndex({ a: 1 }),
expectedReturnVal: 'a_1'
},
{
name: 'createCollection',
command: client => client.db('test').createCollection('test'),
expectedReturnVal: {}
}
];

for (const op of writeOperations) {
context(`when the write operation ${op.name} is run`, function () {
let client;
let spy;

beforeEach(async function () {
if (gte('3.6.0', this.configuration.version)) {
this.currentTest.skipReason = 'Test requires OP_MSG, needs to be on MongoDB 3.6+';
this.skip();
}
spy = sinon.spy(OpMsgRequest.prototype, 'toBin');
client = this.configuration.newClient({ monitorCommands: true, w: 0 });
await client.connect();
});

afterEach(function () {
sinon.restore();
client.close();
});

it('the request should have moreToCome bit set', async function () {
await op.command(client);
expect(spy.returnValues[spy.returnValues.length - 1][0][16]).to.equal(2);
});

it('the return value of the command should be nullish', async function () {
const result = await op.command(client);
expect(result).to.containSubset(op.expectedReturnVal);
});

it('commandSucceededEvent should have reply with only {ok: 1}', async function () {
const events: CommandSucceededEvent[] = [];
client.on('commandSucceeded', event => events.push(event));
await op.command(client);
expect(events[0]).to.containSubset({ reply: { ok: 1 } });
});
});
}
});
});
});
11 changes: 11 additions & 0 deletions test/unit/commands.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,14 @@ describe('class OpCompressedRequest', () => {
}
});
});

describe('OpMsgRequest', () => {
describe('#constructor', () => {
context('when writeConcern = 0', () => {
it('moreToCome is set to true', async () => {
const request = new OpMsgRequest('db', { a: 1, writeConcern: { w: 0 } }, {});
expect(request.moreToCome).to.be.true;
});
});
});
});