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(CSOT) - feature branch #4095

Open
wants to merge 71 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
71 commits
Select commit Hold shift + click to select a range
ab21b5b
Install timeout throughout operation layer
W-A-James Apr 11, 2024
2338dff
update with timeout
W-A-James Apr 24, 2024
9615138
start prose test impl
W-A-James Apr 24, 2024
0eb1986
add timeout to find.execute
W-A-James Apr 26, 2024
69f1087
start implementing prose tests
W-A-James Apr 26, 2024
ff2ec44
don't construct Timeout when not needed
W-A-James Apr 26, 2024
a3e552a
ensure that timeoutMS is passed down correctly
W-A-James Apr 26, 2024
a9c96ac
start working on unit tests
W-A-James Apr 26, 2024
21b2fcd
continue prose test implementation
W-A-James Apr 26, 2024
892800d
revert spec test changes
W-A-James Apr 26, 2024
6f62c9f
revert spec test changes
W-A-James Apr 26, 2024
b878c6c
revert spec test changes
W-A-James Apr 26, 2024
c5fae5c
support timeout on run_command
W-A-James May 1, 2024
124f2d7
continue prose test implementation
W-A-James May 1, 2024
cd6a27e
prose test changes
W-A-James May 1, 2024
b63b63f
WIP - server selection changes
W-A-James May 1, 2024
9f4e884
revert unneeded connection changes
W-A-James May 3, 2024
6fc1198
add serverSelectionTimeout to run_command
W-A-James May 3, 2024
c18b7f0
Merge branch 'main' into NODE-6090
W-A-James May 3, 2024
9c40f94
use correct timeout
W-A-James May 3, 2024
a958ad8
Merge branch 'NODE-6090' of github.com:mongodb/node-mongodb-native in…
W-A-James May 3, 2024
a75d9df
reorder operations
W-A-James May 3, 2024
875ea67
formatting
W-A-James May 3, 2024
3839330
skip some CSOT tests that cannot be made to pass here
W-A-James May 3, 2024
c738691
Improve timeout messages
W-A-James May 3, 2024
c4ae2fb
silence eslint test issues
W-A-James May 3, 2024
bf5a37a
bump timeout values
W-A-James May 3, 2024
e2f9125
misc changes
W-A-James May 3, 2024
472fa9e
rename timeout
W-A-James May 3, 2024
43e69bd
make getter internal
W-A-James May 3, 2024
fb96314
rename timeout
W-A-James May 3, 2024
ad55e9c
remove unneeded change for this PR
W-A-James May 3, 2024
6c7adf1
clear server selection timeout after checkout and remove command exec…
W-A-James May 6, 2024
f586736
move Timeout.min to independent helper function
W-A-James May 6, 2024
021a94d
move Timeout.min to independent helper function
W-A-James May 6, 2024
a61a9d0
Merge branch 'main' into NODE-6090
W-A-James May 7, 2024
30e564c
update timeout propagation
W-A-James May 7, 2024
02509a1
clean up
W-A-James May 7, 2024
7c73c58
cleanup
W-A-James May 7, 2024
b92162b
test cleanup
W-A-James May 7, 2024
4625af4
clean up
W-A-James May 7, 2024
7d7f005
simplify calculation
W-A-James May 7, 2024
ecdd66d
cleanup
W-A-James May 7, 2024
9307a80
clarify branching timeout behaviour
W-A-James May 7, 2024
78ccbd8
Merge branch 'main' into NODE-6090
aditi-khare-mongoDB May 8, 2024
c4d26c5
operationTimeout -> timeout
W-A-James May 8, 2024
3590e5e
Merge branch 'NODE-6090' of github.com:mongodb/node-mongodb-native in…
W-A-James May 8, 2024
f7cc3a5
ensure timeouts are properly cleared
W-A-James May 8, 2024
a1c7601
don't race if given infinite timeout
W-A-James May 8, 2024
2ed34ac
default clearTimeout to false
W-A-James May 9, 2024
7111909
remove clearTimeout variable
W-A-James May 9, 2024
86a7eeb
conditionally clear timeout on early return
W-A-James May 9, 2024
2a816e3
fix unit tests
W-A-James May 9, 2024
62bb3ca
bump test timeout value
W-A-James May 9, 2024
cdce963
replace test with sinon fake timer test
W-A-James May 10, 2024
5c91311
Update src/sdam/topology.ts
W-A-James May 10, 2024
72c22b9
clean up logic
W-A-James May 10, 2024
68f1eec
Update test to assert on current behaviour
W-A-James May 10, 2024
e88191a
Merge branch 'main' into NODE-6090
W-A-James May 23, 2024
daf0d5a
fix autoconnect
W-A-James May 23, 2024
1d9ac3e
add test
W-A-James May 23, 2024
e468b54
remove .only
W-A-James May 23, 2024
fd6c751
fix test
W-A-James May 24, 2024
bab1667
Merge branch 'main' into NODE-6090
W-A-James May 24, 2024
4d126eb
ensure test only runs when failcommand is available
W-A-James May 24, 2024
92564d8
do not run on 4.2
W-A-James May 28, 2024
09a8b17
fix csot test?
W-A-James May 28, 2024
f2f4d63
Merge branch 'main' into NODE-6090
W-A-James May 29, 2024
c4e6ab0
add failpoint to correct commands
W-A-James May 29, 2024
cd6991f
Merge branch 'main' into NODE-6090
W-A-James May 29, 2024
d44b479
resolve merge conflict
W-A-James May 29, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 20 additions & 18 deletions src/cmap/connection_pool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,8 @@ export interface ConnectionPoolOptions extends Omit<ConnectionOptions, 'id' | 'g
export interface WaitQueueMember {
resolve: (conn: Connection) => void;
reject: (err: AnyError) => void;
timeout: Timeout;
clearTimeout: boolean;
timeout: Timeout | null;
clearTimeout: boolean | null;
[kCancelled]?: boolean;
}

Expand Down Expand Up @@ -367,19 +367,21 @@ export class ConnectionPool extends TypedEventEmitter<ConnectionPoolEvents> {

const { promise, resolve, reject } = promiseWithResolvers<Connection>();

let timeout: Timeout;
let clearTimeout: boolean;
let timeout: Timeout | null = null;
let clearTimeout: boolean | null = null;
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
if (options?.timeout) {
// CSOT enabled
// Determine if we're using the timeout passed in or a new timeout
if (
csotMin(options.timeout.duration, serverSelectionTimeoutMS) === serverSelectionTimeoutMS
) {
timeout = Timeout.expires(serverSelectionTimeoutMS - options.timeout.timeElapsed);
clearTimeout = true;
} else {
timeout = options.timeout;
clearTimeout = false;
if (options.timeout.duration > 0 || serverSelectionTimeoutMS > 0) {
if (
csotMin(options.timeout.duration, serverSelectionTimeoutMS) === serverSelectionTimeoutMS
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still and equals check, sorry I think we discussed it but didn't leave a comment, if duration is the same then we'll create a new timeout when we can use the existing one.

) {
timeout = Timeout.expires(serverSelectionTimeoutMS - options.timeout.timeElapsed);
clearTimeout = true;
} else {
timeout = options.timeout;
clearTimeout = false;
}
}
} else {
timeout = Timeout.expires(waitQueueTimeoutMS);
Expand All @@ -397,8 +399,8 @@ export class ConnectionPool extends TypedEventEmitter<ConnectionPoolEvents> {
process.nextTick(() => this.processWaitQueue());

try {
waitQueueMember.timeout.throwIfExpired();
return await Promise.race([promise, waitQueueMember.timeout]);
timeout?.throwIfExpired();
return await (timeout ? Promise.race([promise, timeout]) : promise);
} catch (error) {
if (TimeoutError.is(error)) {
waitQueueMember[kCancelled] = true;
Expand All @@ -422,7 +424,7 @@ export class ConnectionPool extends TypedEventEmitter<ConnectionPoolEvents> {
}
throw error;
} finally {
if (clearTimeout) timeout.clear();
if (clearTimeout) timeout?.clear();
}
}

Expand Down Expand Up @@ -787,7 +789,7 @@ export class ConnectionPool extends TypedEventEmitter<ConnectionPoolEvents> {
ConnectionPool.CONNECTION_CHECK_OUT_FAILED,
new ConnectionCheckOutFailedEvent(this, reason, error)
);
if (waitQueueMember.clearTimeout) waitQueueMember.timeout.clear();
if (waitQueueMember.clearTimeout) waitQueueMember.timeout?.clear();
this[kWaitQueue].shift();
waitQueueMember.reject(error);
continue;
Expand All @@ -808,7 +810,7 @@ export class ConnectionPool extends TypedEventEmitter<ConnectionPoolEvents> {
ConnectionPool.CONNECTION_CHECKED_OUT,
new ConnectionCheckedOutEvent(this, connection)
);
if (waitQueueMember.clearTimeout) waitQueueMember.timeout.clear();
if (waitQueueMember.clearTimeout) waitQueueMember.timeout?.clear();

this[kWaitQueue].shift();
waitQueueMember.resolve(connection);
Expand Down Expand Up @@ -847,7 +849,7 @@ export class ConnectionPool extends TypedEventEmitter<ConnectionPoolEvents> {
waitQueueMember.resolve(connection);
}

if (waitQueueMember.clearTimeout) waitQueueMember.timeout.clear();
if (waitQueueMember.clearTimeout) waitQueueMember.timeout?.clear();
}
process.nextTick(() => this.processWaitQueue());
});
Expand Down
40 changes: 21 additions & 19 deletions src/sdam/topology.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,8 @@ export interface ServerSelectionRequest {
resolve: (server: Server) => void;
reject: (error: MongoError) => void;
[kCancelled]?: boolean;
clearTimeout: boolean;
timeout: Timeout;
clearTimeout: boolean | null;
timeout: Timeout | null;
operationName: string;
waitingLogged: boolean;
previousServer?: ServerDescription;
Expand Down Expand Up @@ -565,19 +565,21 @@ export class Topology extends TypedEventEmitter<TopologyEvents> {
);
}
const serverSelectionTimeoutMS = options.serverSelectionTimeoutMS ?? 0;
let timeout: Timeout;
let clearTimeout: boolean;
let timeout: Timeout | null = null;
let clearTimeout: boolean | null = null;
if (options.timeout) {
// CSOT Enabled
if (
Math.min(options.timeout.remainingTime, serverSelectionTimeoutMS) ===
serverSelectionTimeoutMS
) {
timeout = Timeout.expires(serverSelectionTimeoutMS);
clearTimeout = true;
} else {
timeout = options.timeout;
clearTimeout = false;
if (options.timeout.duration !== 0 || serverSelectionTimeoutMS !== 0) {
if (
Math.min(options.timeout.remainingTime, serverSelectionTimeoutMS) ===
serverSelectionTimeoutMS
) {
timeout = Timeout.expires(serverSelectionTimeoutMS);
clearTimeout = true;
} else {
timeout = options.timeout;
clearTimeout = false;
}
}
} else {
timeout = Timeout.expires(serverSelectionTimeoutMS);
Expand Down Expand Up @@ -629,8 +631,8 @@ export class Topology extends TypedEventEmitter<TopologyEvents> {
processWaitQueue(this);

try {
timeout.throwIfExpired();
return await Promise.race([serverPromise, waitQueueMember.timeout]);
timeout?.throwIfExpired();
return await (timeout ? Promise.race([serverPromise, timeout]) : serverPromise);
} catch (error) {
if (TimeoutError.is(error)) {
// Timeout
Expand Down Expand Up @@ -666,7 +668,7 @@ export class Topology extends TypedEventEmitter<TopologyEvents> {
// Other server selection error
throw error;
} finally {
if (clearTimeout) timeout.clear();
if (clearTimeout) timeout?.clear();
}
}
/**
Expand Down Expand Up @@ -924,7 +926,7 @@ function drainWaitQueue(queue: List<ServerSelectionRequest>, drainError: MongoDr
continue;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

drainWaitQueue is called when the topology is closed. Is it correct not to clear timeouts when the client is closed? I don't think so.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or do we rely on drainWaitQueue rejecting each request in the wait queue, which would clear the timeout when the catch handler is run in selectServer?


if (waitQueueMember.clearTimeout) waitQueueMember.timeout.clear();
if (waitQueueMember.clearTimeout) waitQueueMember.timeout?.clear();

if (!waitQueueMember[kCancelled]) {
if (
Expand Down Expand Up @@ -979,7 +981,7 @@ function processWaitQueue(topology: Topology) {
)
: serverDescriptions;
} catch (selectorError) {
if (waitQueueMember.clearTimeout) waitQueueMember.timeout.clear();
if (waitQueueMember.clearTimeout) waitQueueMember.timeout?.clear();
if (
topology.client.mongoLogger?.willLog(
MongoLoggableComponent.SERVER_SELECTION,
Expand Down Expand Up @@ -1067,7 +1069,7 @@ function processWaitQueue(topology: Topology) {
transaction.pinServer(selectedServer);
}

if (waitQueueMember.clearTimeout) waitQueueMember.timeout.clear();
if (waitQueueMember.clearTimeout) waitQueueMember.timeout?.clear();

if (
topology.client.mongoLogger?.willLog(
Expand Down