Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

@lpinca
Copy link
Member

@lpinca lpinca commented Jan 15, 2019

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
Copy link
Collaborator

nodejs-github-bot commented Jan 15, 2019

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

@nodejs-github-bot nodejs-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
doc/api/tls.md Outdated
Copy link
Contributor

@cjihrig cjihrig Jan 15, 2019

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.

Copy link
Member Author

@lpinca lpinca Jan 15, 2019

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
doc/api/tls.md Outdated
Copy link
Contributor

@sam-github sam-github Jan 15, 2019

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?

@richardlau richardlau mentioned this pull request Jan 15, 2019
Closed
@lpinca lpinca force-pushed the add/timeout-option branch from ef94920 to cad7708 Compare January 15, 2019 17:05
@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.
@lpinca lpinca force-pushed the add/timeout-option branch from cad7708 to 7c158bf Compare January 15, 2019 17:07
silverwind
silverwind reviewed Jan 15, 2019
View reviewed changes
lib/_tls_wrap.js
lookup: options.lookup
};

if (options.timeout) {
Copy link
Contributor

@silverwind silverwind Jan 15, 2019

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.

Copy link
Member Author

@lpinca lpinca Jan 15, 2019 •
edited
Loading

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.

Copy link
Contributor

@silverwind silverwind Jan 15, 2019

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.

Copy link
Contributor

@sam-github sam-github Jan 15, 2019

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
Copy link
Member Author

lpinca commented Jan 16, 2019

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

@lpinca
Copy link
Member Author

lpinca commented Jan 20, 2019

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

@lpinca
Copy link
Member Author

lpinca commented Jan 20, 2019

Landed in aaa7547.

@lpinca lpinca 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 <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 24, 2019
@MylesBorins MylesBorins 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
@lpinca lpinca 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 <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BethGriggs BethGriggs 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 <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
@Nyaran Nyaran mentioned this pull request Aug 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjihrig cjihrig cjihrig left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

+2 more reviewers

@sam-github sam-github sam-github approved these changes

@silverwind silverwind silverwind approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

semver-minor PRs that contain new features and should be released in the next minor version. tls Issues 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