Conversation
tadjik1
force-pushed
the
NODE-7459
branch
2 times, most recently
from
b3c05d1 to
01445e8
Compare
| socket = net.createConnection(parseConnectOptions(options)); | ||
| } | ||
|
|
||
| // Explicit setKeepAlive/setNoDelay are required because tls.connect() silently |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We can add a check that the returned socket has the configured value (for Symbol('kSetKeepAliveInitialDelay')), but that then ties us to the Node implementation and the test can fail for all sorts of new reasons.
Maybe add a strongly-worded comment about keeping this code even if we think it's safe to remove?
3 participants
Description
Summary of Changes
Revert previous behaviour of explicitly calling setKeepAlive and setNoDelay on the socket due to the Node.js bug: nodejs/node#62004
Notes for Reviewers
What is the motivation for this change?
Release Highlight
Release notes highlight
Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript