feat(NODE-5189): deprecate tcp keepalive options #3621
Conversation
nbbeeken
added
the
Primary Review
There was a problem hiding this comment.
@durran No concerns with the deprecations (as long as there is also a clear ticket to remove the options), but changing the defaults is arguably breaking and I think that's why we framed NODE-3607 as a spike; can we take this back and discuss it as a team? I didn't see a kickoff comment or a link to an external doc summarizing the results of the investigation, but some notes addressing the 6 spec callouts point by point would help figure out what changes can happen now and which ones need a follow up ticket.
src/connection_string.ts
Outdated
| @@ -864,12 +864,14 @@ export const OPTIONS = { | |||
| return wc; | |||
| } | |||
| }, | |||
| /** @deprecated - Will not be able to turn off in the future. */ | |||
There was a problem hiding this comment.
We have the deprecated: true property that we can add to the option description objects here to get warnings.
There was a problem hiding this comment.
Added option with message.
src/cmap/connect.ts
Outdated
| // Default to delay to 300 seconds. Node automatically then sets TCP_KEEPINTVL to 1 second | ||
| // which is acceptable to the recommendation of 10 seconds and also cannot be configured. | ||
| // TCP_KEEPCNT is also set to 10 in Node and cannot be configured. (Recommendation is 9) | ||
| const keepAliveInitialDelay = options.keepAliveInitialDelay || 300000; |
There was a problem hiding this comment.
| const keepAliveInitialDelay = options.keepAliveInitialDelay || 300000; | |
| const keepAliveInitialDelay = options.keepAliveInitialDelay; |
Because of the defaulting, we should always have a defined number here.
I've update the NODE-3607 JIRA ticket in the comments with notes for each point in the spec. |
dariakp
changed the title
durran
changed the title
src/cmap/connect.ts
Outdated
| // Default to delay to 300 seconds. Node automatically then sets TCP_KEEPINTVL to 1 second | ||
| // which is acceptable to the recommendation of 10 seconds and also cannot be configured. | ||
| // TCP_KEEPCNT is also set to 10 in Node and cannot be configured. (Recommendation is 9) | ||
| const keepAliveInitialDelay = options.keepAliveInitialDelay; |
There was a problem hiding this comment.
We only want to do the deprecations; changing the defaults would be a potentially breaking change (at any rate it would change behavior), so we can do that in NODE-5190.
There was a problem hiding this comment.
Removed the changes, only deprecations now.
nbbeeken
added
Team Review
Description
Updates TCP keepalive options to the spec where appropriate.
What is changing?
keepAliveandkeepAliveInitialDelayoptions.keepAliveInitialDelayto 300 seconds.Node does not allow fine tuning of the keepalive options as per the spec, so we set the initial delay and then accept the other settings that Node sets internally. This results in:
SO_KEEPALIVE=1TCP_KEEPIDLE=300000TCP_KEEPCNT=10TCP_KEEPINTVL=1Is there new documentation needed for these changes?
mongodb/docs-node#639
What is the motivation for this change?
NODE-3607
Double check the following
npm run check:lintscripttype(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript