Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 18 additions & 1 deletion src/cmap/connect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,15 @@ export async function prepareHandshakeDocument(
return handshakeDoc;
}

/**
* @internal
* Default TCP keepAlive initial delay in milliseconds.
* Set to half the Azure load balancer idle timeout (240s) to ensure
* probes fire well before cloud LBs (Azure, AWS PrivateLink/NLB)
* drop idle connections.
*/
export const DEFAULT_KEEP_ALIVE_INITIAL_DELAY_MS = 120_000;

/** @public */
export const LEGAL_TLS_SOCKET_OPTIONS = [
'allowPartialTrustChain',
Expand Down Expand Up @@ -324,7 +333,7 @@ function parseConnectOptions(options: ConnectionOptions): SocketConnectOpts {
(result as Document)[name] = options[name];
}
}
result.keepAliveInitialDelay ??= 120000;
result.keepAliveInitialDelay ??= DEFAULT_KEEP_ALIVE_INITIAL_DELAY_MS;
result.keepAlive = true;
result.noDelay = options.noDelay ?? true;

Expand Down Expand Up @@ -370,6 +379,9 @@ export async function makeSocket(options: MakeConnectionOptions): Promise<Stream
const useTLS = options.tls ?? false;
const connectTimeoutMS = options.connectTimeoutMS ?? 30000;
const existingSocket = options.existingSocket;
const keepAliveInitialDelay =
options.keepAliveInitialDelay ?? DEFAULT_KEEP_ALIVE_INITIAL_DELAY_MS;
const noDelay = options.noDelay ?? true;

let socket: Stream;

Expand All @@ -396,6 +408,11 @@ export async function makeSocket(options: MakeConnectionOptions): Promise<Stream
socket = net.createConnection(parseConnectOptions(options));
}

// Explicit setKeepAlive/setNoDelay are required because tls.connect() silently
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anything we can do to test that the actual tls.connect() does not ignore the constructor option? The unit tests we are adding to ensure that we test the setKeepAlive call are fine, but imagine a new engineer coming across this comment in a couple of years, following the nodejs issue link, seeing that it's fixed in all relevant node versions, and then removing this logic that's now redundant with the constructor options. And then imagine this bug getting reintroduced in a later node version - it would be nice if we had a regression test that would alert us if that happened.

// ignores these constructor options due to a Node.js bug.
// See: https://github.com/nodejs/node/issues/62003
socket.setKeepAlive(true, keepAliveInitialDelay);
socket.setNoDelay(noDelay);
socket.setTimeout(connectTimeoutMS);

let cancellationHandler: ((err: Error) => void) | null = null;
Expand Down
107 changes: 107 additions & 0 deletions test/unit/cmap/connect.test.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,23 @@
import { expect } from 'chai';
import * as fs from 'fs';
import * as net from 'net';
import * as path from 'path';
import * as process from 'process';
import * as sinon from 'sinon';
import * as tls from 'tls';

import {
CancellationToken,
type ClientMetadata,
connect,
type Connection,
type ConnectionOptions,
DEFAULT_KEEP_ALIVE_INITIAL_DELAY_MS,
HostAddress,
isHello,
LEGACY_HELLO_COMMAND,
makeClientMetadata,
makeSocket,
MongoClientAuthProviders,
MongoCredentials,
MongoNetworkError,
Expand Down Expand Up @@ -448,4 +455,104 @@ describe('Connect Tests', function () {
});
});
});

describe('makeSocket', function () {
let tlsServer: tls.Server;
let tlsPort: number;
let setKeepAliveSpy: sinon.SinonSpy;
let setNoDelaySpy: sinon.SinonSpy;

const serverPem = fs.readFileSync(
path.join(__dirname, '../../integration/auth/ssl/server.pem')
);

before(function (done) {
// @SECLEVEL=0 allows the legacy test certificate (signed with SHA-1/1024-bit RSA)
// to be accepted by OpenSSL 3.x, which rejects at the default security level.
tlsServer = tls.createServer(
{ key: serverPem, cert: serverPem, ciphers: 'DEFAULT:@SECLEVEL=0' },
() => {
/* empty */
}
);
tlsServer.listen(0, '127.0.0.1', () => {
tlsPort = (tlsServer.address() as net.AddressInfo).port;
done();
});
});

after(function () {
tlsServer?.close();
});

beforeEach(function () {
setKeepAliveSpy = sinon.spy(net.Socket.prototype, 'setKeepAlive');
setNoDelaySpy = sinon.spy(net.Socket.prototype, 'setNoDelay');
});

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

context('when tls is enabled', function () {
it('calls setKeepAlive with default keepAliveInitialDelay', async function () {
const socket = await makeSocket({
hostAddress: new HostAddress(`127.0.0.1:${tlsPort}`),
tls: true,
rejectUnauthorized: false,
ciphers: 'DEFAULT:@SECLEVEL=0'
} as ConnectionOptions);
socket.destroy();

expect(setKeepAliveSpy).to.have.been.calledWith(true, DEFAULT_KEEP_ALIVE_INITIAL_DELAY_MS);
});

it('calls setKeepAlive with custom keepAliveInitialDelay', async function () {
const socket = await makeSocket({
hostAddress: new HostAddress(`127.0.0.1:${tlsPort}`),
tls: true,
rejectUnauthorized: false,
ciphers: 'DEFAULT:@SECLEVEL=0',
keepAliveInitialDelay: 5000
} as ConnectionOptions);
socket.destroy();

expect(setKeepAliveSpy).to.have.been.calledWith(true, 5000);
});

it('calls setNoDelay with true by default', async function () {
const socket = await makeSocket({
hostAddress: new HostAddress(`127.0.0.1:${tlsPort}`),
tls: true,
rejectUnauthorized: false,
ciphers: 'DEFAULT:@SECLEVEL=0'
} as ConnectionOptions);
socket.destroy();

expect(setNoDelaySpy).to.have.been.calledWith(true);
});
});

context('when tls is disabled', function () {
it('calls setKeepAlive with default keepAliveInitialDelay', async function () {
const socket = await makeSocket({
hostAddress: new HostAddress(`127.0.0.1:${tlsPort}`),
tls: false
} as ConnectionOptions);
socket.destroy();

expect(setKeepAliveSpy).to.have.been.calledWith(true, DEFAULT_KEEP_ALIVE_INITIAL_DELAY_MS);
});

it('calls setNoDelay with true by default', async function () {
const socket = await makeSocket({
hostAddress: new HostAddress(`127.0.0.1:${tlsPort}`),
tls: false
} as ConnectionOptions);
socket.destroy();

expect(setNoDelaySpy).to.have.been.calledWith(true);
});
});
});
});