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.

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

doc: update http.md for consistency #10715

Closed
wants to merge 9 commits into from
Closed

doc: update http.md for consistency #10715

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f9c053d
doc: update http.md for consistency
lance Jan 9, 2017
cfeedac
doc: incorporate feedback into http.md
lance Jan 9, 2017
a776624
doc: attempt to clarify behavior of http.Agent
lance Jan 17, 2017
e92d4ae
doc: minor wording change/clarification to http.md
lance Jan 17, 2017
4d7148b
doc: clarify that pooled sockets are unref()ed.
lance Jan 18, 2017
bf0b7d1
doc: fix wording for agent option in http.request
lance Jan 18, 2017
dab4842
doc: fix link to net.html#net_socket_unref
lance Jan 18, 2017
8972714
doc: minor adjustments to keepAlive usage
lance Jan 18, 2017
05a8bcc
doc: incorporate feedback on http.Agent docs
lance Jan 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 28 additions & 28 deletions doc/api/http.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,25 +48,25 @@ list like the following:
added: v0.3.4
-->

The HTTP Agent is used for pooling sockets used in HTTP client
The HTTP `Agent` is used for pooling sockets used in HTTP client
requests.

The HTTP Agent also defaults client requests to using
The HTTP `Agent` also defaults client requests to using
`Connection: keep-alive`. If no pending HTTP requests are waiting on a
socket to become free the socket is closed. This means that Node.js's
pool has the benefit of keep-alive when under load but still does not
require developers to manually close the HTTP clients using
KeepAlive.

If you opt into using HTTP KeepAlive, you can create an Agent object
with that flag set to `true`. (See the [constructor options][].)
Then, the Agent will keep unused sockets in a pool for later use. They
will be explicitly marked so as to not keep the Node.js process running.
However, it is still a good idea to explicitly [`destroy()`][] KeepAlive
agents when they are no longer in use, so that the Sockets will be shut
down.

Sockets are removed from the agent's pool when the socket emits either
pool has the benefit of HTTP Keep-Alive when under load but still does not
require developers to manually close HTTP clients when using
this option.

If you opt into using HTTP Keep-Alive, you can create an `Agent` instance,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so "HTTP Keep-Alive" is different from "Connection: keep-alive"? I thought that latter was an HTTP header, and is HTTP keep-alive? But you said keep alive is the default above, but here you say you have to opt-in. I'm confused.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

read more, I think what you are calling "HTTP Keep-Alive" here would be better described as "Agent Keep-Alive", or something of the sort, its a purely Agent thing to keep HTTP connections around, even when unused. Unlike HTTP Keep-Alive, which is to allow multiple requests over a single TCP socket.

Copy link
Member Author

@lance lance Jan 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are valid points. To be clear, for the most part, I haven't changed the body of the text beyond trying to be consistent about styling, hyphenation and capitalization. These two or three confusing paragraphs definitely need work. To be honest, I didn't spend a huge amount of time reading the body of the text for overall context. Rather, I was looking for punctuation, styling and capitalization inconsistencies.

If it makes sense to improve the clarity of this content as you describe here, I can think on it and add it to the PR. But that was not really the original purpose here.

providing `{ keepAlive: true }` among the constructor options. (See the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just link 'constructor options' here instead of having the extra '(See the constructor options.)'

[constructor options][].) The `Agent` will then keep unused sockets in a
Copy link
Contributor

@mscdex mscdex Jan 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/`Agent`/agent/

pool for later use. They will be explicitly marked so as to not keep the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps 'They' could be clarified here? It's not immediately clear if it's referring to the Agent instance or the unused sockets.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the socket that is unrefed here, right? Not the agent itself. Which also makes me wonder if unref should be mentioned here, or if that's too much detail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably just replace 'They' with 'The unused sockets'.

Node.js process running. However, it is still a good idea to explicitly
[`destroy()`][] HTTP Keep-Alive agents when they are no longer in use, so that
the Sockets will be shut down.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Sockets/sockets/


Sockets are removed from the `Agent`'s pool when the socket emits either
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/the `Agent`'s/an agent's/

a `'close'` event or a special `'agentRemove'` event. This means that if
you intend to keep one HTTP request open for a long time and don't
want it to stay in the pool you can do something along the lines of:
Expand Down Expand Up @@ -136,7 +136,7 @@ added: v0.11.4
Produces a socket/stream to be used for HTTP requests.

By default, this function is the same as [`net.createConnection()`][]. However,
custom Agents may override this method in case greater flexibility is desired.
custom agents may override this method in case greater flexibility is desired.

A socket/stream can be supplied in one of two ways: by returning the
socket/stream from this function, or by passing the socket/stream to `callback`.
Expand All @@ -151,7 +151,7 @@ added: v0.11.4
Destroy any sockets that are currently in use by the agent.

It is usually not necessary to do this. However, if you are using an
agent with KeepAlive enabled, then it is best to explicitly shut down
agent with HTTP Keep-Alive enabled, then it is best to explicitly shut down
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be keepAlive (HTTP Keep-Alive is always used, remember, Connection: keep-alive IS "HTTP Keep-Alive")

the agent when you know that it will no longer be used. Otherwise,
sockets may hang open for quite a long time before the server
terminates them.
Expand All @@ -164,7 +164,7 @@ added: v0.11.4
* {Object}

An object which contains arrays of sockets currently awaiting use by
the Agent when HTTP KeepAlive is used. Do not modify.
the agent when HTTP Keep-Alive is used. Do not modify.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keepAlive is enabled.


### agent.getName(options)
<!-- YAML
Expand All @@ -179,8 +179,8 @@ added: v0.11.4
* Returns: {String}

Get a unique name for a set of request options, to determine whether a
connection can be reused. In the http agent, this returns
`host:port:localAddress`. In the https agent, the name includes the
connection can be reused. In the HTTP agent, this returns
Copy link
Contributor

@mscdex mscdex Jan 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/In the HTTP/For an HTTP/

`host:port:localAddress`. In the HTTPS agent, the name includes the
Copy link
Contributor

@mscdex mscdex Jan 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/In the HTTPS/For an HTTPS/

CA, cert, ciphers, and other HTTPS/TLS-specific options that determine
socket reusability.

Expand All @@ -191,7 +191,7 @@ added: v0.11.7

* {Number}

By default set to 256. For Agents supporting HTTP KeepAlive, this
By default set to 256. For agents supporting HTTP Keep-Alive, this
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keepAlive

sets the maximum number of sockets that will be left open in the free
state.

Expand Down Expand Up @@ -224,7 +224,7 @@ added: v0.3.6
* {Object}

An object which contains arrays of sockets currently in use by the
Agent. Do not modify.
agent. Do not modify.

## Class: http.ClientRequest
<!-- YAML
Expand Down Expand Up @@ -652,7 +652,7 @@ added: v0.1.0
* `response` {http.ServerResponse}

Emitted each time there is a request. Note that there may be multiple requests
per connection (in the case of keep-alive connections).
per connection (in the case of HTTP Keep-Alive connections).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct usage

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're saying this is the correct usage, right? Not that I should correct the usage here as well. I think this has the intended meaning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks correct to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry, I am saying this is the correct usage, in contrast to a number of other places where I commented that it wasn't.


### Event: 'upgrade'
<!-- YAML
Expand Down Expand Up @@ -1490,7 +1490,7 @@ added: v0.5.9

* {http.Agent}

Global instance of Agent which is used as the default for all HTTP client
Global instance of `Agent` which is used as the default for all HTTP client
requests.

## http.request(options[, callback])
Expand Down Expand Up @@ -1520,15 +1520,15 @@ added: v0.3.6
* `headers` {Object} An object containing request headers.
* `auth` {String} Basic authentication i.e. `'user:password'` to compute an
Authorization header.
* `agent` {http.Agent|Boolean} Controls [`Agent`][] behavior. When an Agent
* `agent` {http.Agent|Boolean} Controls [`Agent`][] behavior. When an `Agent`
is used request will default to `Connection: keep-alive`. Possible values:
* `undefined` (default): use [`http.globalAgent`][] for this host and port.
* `Agent` object: explicitly use the passed in `Agent`.
* `false`: opts out of connection pooling with an Agent, defaults request to
* `false`: opts out of connection pooling with an `Agent`, defaults request to
`Connection: close`.
* `createConnection` {Function} A function that produces a socket/stream to
use for the request when the `agent` option is not used. This can be used to
avoid creating a custom Agent class just to override the default
use for the request when the `Agent` option is not used. This can be used to
Copy link
Contributor

@mscdex mscdex Jan 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be changed, it's referring to the agent http.request() option above.

avoid creating a custom `Agent` class just to override the default
`createConnection` function. See [`agent.createConnection()`][] for more
details.
* `timeout` {Integer}: A number specifying the socket timeout in milliseconds.
Expand Down