Skip to content
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

https agent docs are incomprehensible #10203

Closed
sam-github opened this issue Dec 9, 2016 · 4 comments
Closed

https agent docs are incomprehensible #10203

sam-github opened this issue Dec 9, 2016 · 4 comments
Labels
doc Issues and PRs related to the documentations. https Issues or PRs related to the https subsystem.

Comments

@sam-github
Copy link
Contributor

sam-github commented Dec 9, 2016

Subsystem: doc,https,http

https://github.com/nodejs/node/blame/d8c7534fcd46283e52c8c9324d11390c6218b809/doc/api/https.md#L195-L232

Docs state that a sub-set of the tls.connect()/secure context options can be provided to https.request().

The following options from [tls.connect()][] can also be specified:

This is almost certainly wrong, probably all of them can be supplied.

The docs then go on to state:

In order to specify these options, use a custom [Agent][].

Its impossible to understand what this means... are they options, or not? Do they only work with custom agents?

There is an example.... but since the example passes the exact same options to a custom agent constructor, and to https.request(), it gets even harder to understand.

It appears that the secure options may not in fact be used as options to https.request()... that you need to provide them to an Agent?

But none of the optoins provided to the Agent constructor in the example are supported by Agent, according to its docs, including the secure context options, so who knows what they do.

So, at the end of reading through the docs:

I have absolutely no idea what secure context options are supported by https.request(), could be all, could be none as the example suggests.

Maybe the docs are completely wrong, and the only way to provide secure context options with https.request() is to use a custom Agent?

Or maybe whether the options argument to https.request() is ignored when you have an agent, and used when you don't?

If anybody knows, feel free to document either here or in a doc PR. I might look at this later, but it wasn't particularly obvious in a quick read.

Also, https://github.com/nodejs/node/blob/master/lib/_http_client.js#L35 is a bit odd, because null is explicitly not documented as an option for the agent property, for http or https. But that's for another issue, I guess.

@sam-github sam-github added doc Issues and PRs related to the documentations. https Issues or PRs related to the https subsystem. labels Dec 9, 2016
@MylesBorins
Copy link
Contributor

/cc @nodejs/http

@fabiancook
Copy link
Contributor

The options passed to https.Agent are defaults to createConnection, which invokes tls.connect
Any options passed to https.request override the defaults

If you are going to do many requests using these options, create the agent using the options and set overrides using https.request

Its probably best to pass options to both the agent and the request

If you are just doing a single request, you shouldn't need to create an agent.

Some thoughts

  • It looks like if you use the socketPath option you need to pass the options to the agent
  • If you are using keepAlive and there isn't a free connection, and you are using secureProtocol, this needs to be either passed consistently between requests, or passed to the agent and not passed for the request
  • Requests using secureProtocol can mix with different secureProtocol socket sessions, unsure if this is an issue or not?

@Trott
Copy link
Member

Trott commented Jul 15, 2017

@nodejs/documentation

@BridgeAR
Copy link
Member

Closing due to lack of participation. I did not check if the docs were updated or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. https Issues or PRs related to the https subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants