Skip to content

Conversation

@baileympearson
Copy link
Contributor

@baileympearson baileympearson commented Nov 19, 2025

Description

Summary of Changes

This PR adopts the backpressure changes from mongodb/specifications#1860 and mongodb/specifications#1855.

Notes for Reviewers

What is the motivation for this change?

Release Highlight

Release notes highlight

Double check the following

  • Lint is passing (npm run check:lint)
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@baileympearson baileympearson force-pushed the NODE-7121 branch 2 times, most recently from f4f55f6 to e14a3ef Compare November 19, 2025 21:16
Comment on lines +445 to +450
const res = server.command(
new RunCommandOperation(ns('admin.$cmd'), { ping: 1 }, {}),
TimeoutContext.create({
serverSelectionTimeoutMS: 30_000,
waitQueueTimeoutMS: 10_000
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typescript wasn't properly inferring the types after command.bind() is called for some reason. After adjusting it to call command directly, I got a compiler error and had to provide a timeout context.

...HEARTBEAT_EVENTS
];

function makeConnectionsError(appError: ApplicationError): sinon.SinonStub[] {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the large diff here, reviewers. Our test runner's old mocks were too permissive and allowed tests to pass that should have failed. I've adjusted this mocking logic:

  • if we need to stub before handshake errors, I create stubs that provide the bare minimum needed for the actual handshake to fail. In practice, this means stubbing net.createConnection().
  • otherwise, I use rely on the old stub logic.

I've also adjusted the code to use switch statements instead of if-statements when switching on discriminated union fields, because TS errors if we haven't exhausted all switch statements.

@baileympearson baileympearson changed the title wip feat(NODE-7121): prevent connection churn on backpressure errors when establishing connections feat(NODE-7121): prevent connection churn on backpressure errors when establishing connections Dec 2, 2025
@baileympearson baileympearson removed the wip label Dec 2, 2025
@baileympearson baileympearson marked this pull request as ready for review December 2, 2025 21:22
@baileympearson baileympearson requested a review from a team as a code owner December 2, 2025 21:22
@tadjik1 tadjik1 self-assigned this Dec 8, 2025
@tadjik1 tadjik1 added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Dec 8, 2025
);

expect(poolClearedEvents).to.be.empty;
expect(checkoutFailedEvents.length).to.be.greaterThan(10);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
expect(checkoutFailedEvents.length).to.be.greaterThan(10);
expect(checkoutFailedEvents.length).to.be.at.least(10);

According to the spec "Assert that at least 10 ConnectionCheckOutFailedEvent occurred."


netStub.callsFake(function createConnectionStub() {
const socket = new net.Socket();
process.nextTick(() => socket.emit('connect'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems using microTask queue won't affect the functionality but it will help us later (less refactoring and unexpected failures)

Suggested change
process.nextTick(() => socket.emit('connect'));
queueMicrotask(() => socket.emit('connect'));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Primary Review In Review with primary reviewer, not yet ready for team's eyes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants