Skip to content

tls: make tls.connect() accept a timeout option - #25517

Closed
lpinca wants to merge 1 commit into
nodejs:masterfrom
lpinca:add/timeout-option
Closed

tls: make tls.connect() accept a timeout option#25517
lpinca wants to merge 1 commit into
nodejs:masterfrom
lpinca:add/timeout-option

Conversation

@lpinca

@lpincalpinca commented Jan 15, 2019

Copy link
Copy Markdown
Member

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.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot

nodejs-github-bot commented Jan 15, 2019

Copy link
Copy Markdown
Collaborator

@lpinca sadly an error occured when I tried to trigger a build :(

@nodejs-github-botnodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Jan 15, 2019
cjihrig
cjihrig reviewed Jan 15, 2019
View reviewed changes
Comment threaddoc/api/tls.md Outdated

@cjihrigcjihrigJan 15, 2019

Copy link
Copy Markdown
Contributor

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.

@lpincalpincaJan 15, 2019

Copy link
Copy Markdown
MemberAuthor

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.

sam-github
sam-github approved these changes Jan 15, 2019
View reviewed changes
Comment threaddoc/api/tls.md Outdated

@sam-githubsam-githubJan 15, 2019

Copy link
Copy Markdown
Contributor

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?

@richardlaurichardlau mentioned this pull request Jan 15, 2019
Closed
@lpinca
tls: make tls.connect() accept a timeout option
7c158bf
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.
silverwind
silverwind reviewed Jan 15, 2019
View reviewed changes
Comment threadlib/_tls_wrap.js
lookup: options.lookup
};

if (options.timeout) {

@silverwindsilverwindJan 15, 2019

Copy link
Copy Markdown
Contributor

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.

@lpincalpincaJan 15, 2019•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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.

@silverwindsilverwindJan 15, 2019

Copy link
Copy Markdown
Contributor

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.

@sam-githubsam-githubJan 15, 2019

Copy link
Copy Markdown
Contributor

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(),

node/lib/net.js

Lines 158 to 160 in 2c7f4f4

if(options.timeout){
socket.setTimeout(options.timeout);
}
, which is what I'd expect, so LGTM as is.

@lpinca

lpinca commented Jan 16, 2019

Copy link
Copy Markdown
MemberAuthor

CI: https://ci.nodejs.org/job/node-test-pull-request/20160/

@lpinca

lpinca commented Jan 20, 2019

Copy link
Copy Markdown
MemberAuthor

New CI: https://ci.nodejs.org/job/node-test-pull-request/20215/

@lpinca

lpinca commented Jan 20, 2019

Copy link
Copy Markdown
MemberAuthor

Landed in aaa7547.

@lpincalpinca closed this Jan 20, 2019
@lpinca
lpinca deleted the add/timeout-option branch January 20, 2019 13:58
lpinca added a commit that referenced this pull request Jan 20, 2019
@lpinca
tls: make tls.connect() accept a timeout option
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>
addaleax pushed a commit that referenced this pull request Jan 23, 2019
@lpinca@addaleax
tls: make tls.connect() accept a timeout option
58952a1
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>
@addaleaxaddaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 24, 2019
@MylesBorinsMylesBorins mentioned this pull request Jan 24, 2019
Merged
MylesBorins added a commit that referenced this pull request Jan 24, 2019
@MylesBorins
2019-01-24, Version 11.8.0 (Current)
11ee794
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
MylesBorins added a commit that referenced this pull request Jan 25, 2019
@MylesBorins
2019-01-24, Version 11.8.0 (Current)
641de82
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
@lpincalpinca mentioned this pull request Mar 4, 2019
Closed
1 task
BethGriggs pushed a commit that referenced this pull request Apr 29, 2019
@lpinca@BethGriggs
tls: make tls.connect() accept a timeout option
f59fe67
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>
@BethGriggsBethGriggs mentioned this pull request May 1, 2019
Merged
BethGriggs pushed a commit that referenced this pull request May 10, 2019
@lpinca@BethGriggs
tls: make tls.connect() accept a timeout option
df04d3b
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>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@lpinca@MylesBorins
tls: make tls.connect() accept a timeout option
2319bc5
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>
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
@NyaranNyaran mentioned this pull request Aug 1, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjihrigcjihrigcjihrig left review comments
@BridgeARBridgeARBridgeAR approved these changes
+2 more reviewers
@sam-githubsam-githubsam-github approved these changes
@silverwindsilverwindsilverwind approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

semver-minorPRs that contain new features and should be released in the next minor version.tlsIssues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@lpinca@nodejs-github-bot@sam-github@silverwind@cjihrig@BridgeAR@addaleax