-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
tls: make tls.connect() accept a timeout option #25517
Conversation
@lpinca sadly an error occured when I tried to trigger a build :( |
doc/api/tls.md
Outdated
@@ -1023,6 +1023,9 @@ being issued by trusted CA (`options.ca`). | |||
<!-- YAML | |||
added: v0.11.3 | |||
changes: | |||
- version: REPLACEME | |||
pr-url: REPLACEME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/nodejs/node/pull/25517
. Just pointing it out because I know it's easy to forget.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes will do just wasn't sure what id to use before submitting.
doc/api/tls.md
Outdated
@@ -1088,6 +1091,9 @@ changes: | |||
`tls.createSecureContext()`. | |||
* `lookup`: {Function} Custom lookup function. **Default:** | |||
[`dns.lookup()`][]. | |||
* `timeout`: {number} If set and if a socket is created internally, will call | |||
`socket.setTimeout(timeout)` after the socket is created, but before it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe hyperlink to the setTimeout docs?
ef94920
to
cad7708
Compare
If specified, and only when a socket is created internally, the option will make `socket.setTimeout()` to be called on the created socket with the given timeout. This is consistent with the `timeout` option of `net.connect()` and prevents the `timeout` option of the `https.Agent` from being ignored when a socket is created.
cad7708
to
7c158bf
Compare
@@ -1256,6 +1256,11 @@ exports.connect = function connect(...args) { | |||
localAddress: options.localAddress, | |||
lookup: options.lookup | |||
}; | |||
|
|||
if (options.timeout) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typeof options.timeout === 'number'
to allow setting a timeout of 0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0 == disabled idle timeout so I'm not sure if it's very useful. I mean it's like not setting it at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, keep it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code for tls.connect() is identical to that in net.connect(),
Lines 158 to 160 in 2c7f4f4
if (options.timeout) { | |
socket.setTimeout(options.timeout); | |
} |
Landed in aaa7547. |
If specified, and only when a socket is created internally, the option will make `socket.setTimeout()` to be called on the created socket with the given timeout. This is consistent with the `timeout` option of `net.connect()` and prevents the `timeout` option of the `https.Agent` from being ignored when a socket is created. PR-URL: #25517 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
If specified, and only when a socket is created internally, the option will make `socket.setTimeout()` to be called on the created socket with the given timeout. This is consistent with the `timeout` option of `net.connect()` and prevents the `timeout` option of the `https.Agent` from being ignored when a socket is created. PR-URL: #25517 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Notable Changes: * events: * For unhandled `error` events with an argument that is not an `Error` object, the resulting exeption will have more information about the argument. #25621 * child_process: * When the `maxBuffer` option is passed, `stdout` and `stderr` will be truncated rather than unavailable in case of an error. #24951 * policy: * Experimental support for module integrity checks through a manifest file is implemented now. #23834 * n-api: * The `napi_threadsafe_function` feature is now stable. #25556 * report: * An experimental diagnostic API for capturing process state is available as `process.report` and through command line flags. #22712 * tls: * `tls.connect()` takes a `timeout` option analogous to the `net.connect()` one. #25517 * worker: * `process.umask()` is available as a read-only function inside Worker threads now. #25526 * An `execArgv` option that supports a subset of Node.js command line options is supported now. #25467 PR-URL: #25687
Notable Changes: * events: * For unhandled `error` events with an argument that is not an `Error` object, the resulting exeption will have more information about the argument. #25621 * child_process: * When the `maxBuffer` option is passed, `stdout` and `stderr` will be truncated rather than unavailable in case of an error. #24951 * policy: * Experimental support for module integrity checks through a manifest file is implemented now. #23834 * n-api: * The `napi_threadsafe_function` feature is now stable. #25556 * report: * An experimental diagnostic API for capturing process state is available as `process.report` and through command line flags. #22712 * tls: * `tls.connect()` takes a `timeout` option analogous to the `net.connect()` one. #25517 * worker: * `process.umask()` is available as a read-only function inside Worker threads now. #25526 * An `execArgv` option that supports a subset of Node.js command line options is supported now. #25467 PR-URL: #25687
If specified, and only when a socket is created internally, the option will make `socket.setTimeout()` to be called on the created socket with the given timeout. This is consistent with the `timeout` option of `net.connect()` and prevents the `timeout` option of the `https.Agent` from being ignored when a socket is created. PR-URL: #25517 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
If specified, and only when a socket is created internally, the option will make `socket.setTimeout()` to be called on the created socket with the given timeout. This is consistent with the `timeout` option of `net.connect()` and prevents the `timeout` option of the `https.Agent` from being ignored when a socket is created. PR-URL: #25517 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
If specified, and only when a socket is created internally, the option will make `socket.setTimeout()` to be called on the created socket with the given timeout. This is consistent with the `timeout` option of `net.connect()` and prevents the `timeout` option of the `https.Agent` from being ignored when a socket is created. PR-URL: #25517 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
If specified, and only when a socket is created internally, the option
will make
socket.setTimeout()
to be called on the created socket withthe given timeout.
This is consistent with the
timeout
option ofnet.connect()
andprevents the
timeout
option of thehttps.Agent
from being ignoredwhen a socket is created.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes