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

doc: update http.md for consistency #10715

Closed
wants to merge 9 commits into from
Closed

doc: update http.md for consistency #10715

wants to merge 9 commits into from

Conversation

lance
Copy link
Member

@lance lance commented Jan 9, 2017

The HTTP Keep-Alive text was inconsistent. These changes follow the
following rules

  • When referring to flag used in code, it should always be written using
    backticks and in camel case. E.g. keepAlive.
  • When referring to the mechanism Keep-Alive functionality as described
    in the HTTP 1.1 RFC, it is written as 'HTTP Keep-Alive', without the
    use of backticks.
  • When referring to the request header, it should always use backticks
    and be written as Connection: keep-alive.

This commit also includes some changes to how http.Agent is
referenced. When Agent is used as a reference to an object or
instance, backticks should always be used.

Left somewhat unresolved is how to determine when to use "agent"
vs. "Agent". At the moment, the API documentation typically uses
"agent" when referring to a generic instance, and either http.Agent or
Agent when referring to a specific instance. But it's not really 100%
clear. I wonder if it would be best to just always use Agent.

Ref: #10614
Fixes: #10567

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

The HTTP Keep-Alive text was inconsistent. These changes follow the
following rules

* When referring to flag used in code, it should always be written using
  backticks and in camel case. E.g. `keepAlive`.
* When referring to the mechanism Keep-Alive functionality as described
  in the HTTP 1.1 RFC, it is written as 'HTTP Keep-Alive', without the
  use of backticks.
* When referring to the request header, it should always use backticks
  and be written as `Connection: keep-alive`.

This commit also includes some changes to how `http.Agent` is
referenced. When `Agent` is used as a reference to an object or
instance, backticks should always be used.

Left somewhat unresolved is how to determine when to use "agent"
vs. "`Agent`". At the moment, the API documentation typically uses
"agent" when referring to a generic instance, and either `http.Agent` or
`Agent` when referring to a specific instance. But it's not really 100%
clear. I wonder if it would be best to just always use `Agent`.

Ref: #10614
Fixes: #10567
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. lts-watch-v6.x labels Jan 9, 2017
@lance lance mentioned this pull request Jan 9, 2017
2 tasks
this option.

If you opt into using HTTP Keep-Alive, you can create an `Agent` instance,
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.)'

If you opt into using HTTP Keep-Alive, you can create an `Agent` instance,
providing `{ keepAlive: true }` among the constructor options. (See the
[constructor options][].) The `Agent` will then keep unused sockets in a
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'.

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()`][] 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/


If you opt into using HTTP Keep-Alive, you can create an `Agent` instance,
providing `{ keepAlive: true }` among the constructor options. (See the
[constructor options][].) The `Agent` will then keep unused sockets in a
Copy link
Contributor

@mscdex mscdex Jan 9, 2017

Choose a reason for hiding this comment

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

s/`Agent`/agent/

[`destroy()`][] HTTP Keep-Alive 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
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/

@@ -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

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/

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
`host:port:localAddress`. In the HTTPS agent, the name includes the
Copy link
Contributor

@mscdex mscdex Jan 9, 2017

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/

`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

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.

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

This needs work:

   * `keepAlive` {Boolean} Keep sockets around in a pool to be used by
     other requests in the future. Default = `false`
   * `keepAliveMsecs` {Integer} When using HTTP KeepAlive, how often

The first bullet's keepAlive is what is referred to in the second bullet, but that is not at all clear.

If the keepAlive option is supposed to mean "HTTP KeepAlive" (note inconsistency in hyphenation, needs fixing), then it should self-describe itself as "HTTP Keep-Alive" so its completely clear!

However, given that there are THREE keep alive behaviours discussed in this PR:

  1. the Agent's, aka "HTTP keep-alive", wherein it keeps unused tcp sockets around for possible use later
  2. HTTP's keep-alive, "Connection: keep-alive", which allows the TCP connection to be reused for more HTTP requests
  3. TCP keep-alive, SOL_KEEPALIVE (better called "kill fast" not "keep alive", but whatever), which causes TCP sockets to be removed faster from the pool if the network or HTTP server becomes unreachable

It needs to crystal clear which one is referred to at all times, they need to be hyphentated consistently, and I submit that using the phrase "HTTP keep alive" for a keep alive that is not, in fact, the one described in the HTTP protocol docs is not a good idea.

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

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.

@lance
Copy link
Member Author

lance commented Jan 9, 2017

@sam-github I agree the new Agent options need work. :)

keepAlive {Boolean} Keep sockets around in a pool to be used by other requests in the future. Default = false

This means both that sockets will be kept around, and also that HTTP Keep-Alive will be active on the sockets. This is somewhat redundant, since with HTTP 1.1, Keep-Alive is active by default and must explicitly be turned off. It's definitely confusing that this single option means both socket pooling AND HTTP Keep-Alive, which itself is unnecessary, but does ultimately result in the SO_KEEPALIVE flag on the underlying socket.

But I don't think adding another option here would make things better. I would say that it's simply a side effect of the flag that HTTP Keep-Alive (and therefore TCP SO_KEEPALIVE) are enabled.

keepAliveMsecs {Integer} When using HTTP KeepAlive, how often to send TCP KeepAlive packets over sockets being kept alive. Default = 1000. Only relevant if keepAlive is set to true.

When using HTTP Keep-Alive as described in this document, the TCP SO_KEEPALIVE flag will be set in lib_uv via tcp_wrap.cc. But I really think it's unnecessary to include all of this detail in the documentation.

How about this:

  • keepAlive {Boolean} Keep sockets around in a pool to be used by other requests in the future. In addition to pooling sockets, this option also enables HTTP Keep-Alive by default on all connections. Default = false
  • keepAliveMsecs {Integer} When using the keepAlive option, this specifies how often to send TCP SO_KEEPALIVE Ack packets. Default = 1000. Only relevant if keepAlive is true.

@sam-github
Copy link
Contributor

sam-github commented Jan 9, 2017

This means both that sockets will be kept around, and also that HTTP Keep-Alive will be active on the sockets.

Agree, and the docs should say so right there (they do not ATM).

But I don't think adding another option here would make things better.

I did not and do not suggest any more options, did you get that impression?

Implementing HTTP Keep Alive does not require setting SOL_KEEPALIVE, and I for one had no idea that causing Connection: keep-alive to be set also had the side effect of setting SOL_KEEPALIVE. If it does that, it needs to be documented.

But I really think it's unnecessary to include all of this detail in the documentation.

I don't agree at all. Setting SOL_KEEPALIVE has observable effects on the sockets, effects that are predictable if we know its set.

Knowing that Connection: keep-alive is set is important for trouble-shooting, and to avoid people asking whether it is or is not set.

My point that "HTTP Keep-Alive" in the docs sometimes refers to https://en.wikipedia.org/wiki/HTTP_persistent_connection ("HTTP keep-alive"), or sometimes to the agent .keepAlive option (and yes, the latter also causes the Connection: keep-alive to be set per-socket).

It needs to be more clear.

@sam-github
Copy link
Contributor

OK, I see where you are coming from, you just want to do a little cleanup. That is OK.

However, from PR description:

When referring to the mechanism Keep-Alive functionality as described
in the HTTP 1.1 RFC, it is written as 'HTTP Keep-Alive', without the
use of backticks.

But from PR:

HTTP Keep-Alive agents when they are no longer in use

Here, HTTP Keep-Alive refers to an agent's pooling behaviour, not to the HTTP RFC, so the terminology is not cleaned up.

@lance
Copy link
Member Author

lance commented Jan 9, 2017

Agree, and the docs should say so righ there (they do not ATM).

So, I can do this - see my earlier note about the original purpose of the PR. I don't mind expanding it, but to be clear this wasn't the original purpose. :)

I did not and do not suggest any more options, did you get that impression?

No not really. I just wanted to be clear about my feelings on it in case we went down that path.

Regarding SO_KEEPALIVE vs. HTTP Keep-Alive, I think it might be useful for someone with more knowledge of the internals to chime in here. I only discovered this connection today because I wanted to understand what was really going on, so I read the code. It may be that I misread, or it may be that the docs need a real overhaul.

@lance
Copy link
Member Author

lance commented Jan 10, 2017

@sam-github after spending even more time reading specs and looking at the existing code, I agree with you both about the fact that the docs are confusing, and also that including low-level detail is relevant and important. I will add some more commits this week that hopefully clarify a bit more.

@sam-github
Copy link
Contributor

@lance thanks for your patience, looking forward to the new docs

@lance
Copy link
Member Author

lance commented Jan 12, 2017

@sam-github Just an update - I have been reading the code all morning, and the keep alive logic is mind boggling, shared between _http_outgoing.js, _http_agent.js, _http_client.js, and net.js. If I'm reading it correctly, as far as clients go, by default, we're not implementing HTTP/1.1 persistence.

The HTTP/1.1 spec says,

A significant difference between HTTP/1.1 and earlier versions of HTTP is that persistent connections are the default behavior of any HTTP connection. That is, unless otherwise indicated, the client SHOULD assume that the server will maintain a persistent connection, even after error responses from the server.

Both the http.globalAgent and the default constructor behave so that multiple, simultaneous requests will reuse an existing connection, but once the Agent instance has no more pending requests for a given host/port, the socket is destroyed, instead of assuming the server will maintain the connection.

But really, I wonder if this is the intended behavior. It's not necessarily broken, but seems like an odd default. That, combined with the fact that the default also does not pool sockets -- you must specify keepAlive: true to enable this -- makes it seems like the Agent doesn't do much on its own, unless expressly created/configured to do so.

To summarize. If I read it correctly, the default behavior for all HTTP client requests using an Agent (even the default http._globalAgent is to:

I would love for someone with better knowledge of the intent to chime in here. My reading of the code could be wrong. But if it's correct, it sure is confusing and I find it hard to believe this is the true intent.

/cc @nodejs/collaborators @nodejs/documentation

@lance lance added the net Issues and PRs related to the net subsystem. label Jan 12, 2017
@lance
Copy link
Member Author

lance commented Jan 12, 2017

Adding the 'net' label for now, since it's not really clear if this is purely a documentation issue.

@mscdex
Copy link
Contributor

mscdex commented Jan 12, 2017

@lance What you're just now discussing (the actual behavior of the http implementation) is unrelated to the original issue, which is the styling of the documentation. So I'm not sure that this (the original change) is related to net.

@lance lance removed the net Issues and PRs related to the net subsystem. label Jan 12, 2017
@lance
Copy link
Member Author

lance commented Jan 12, 2017

@mscdex fair enough. Label removed. Do you think what I've described here is worthy of a separate issue? And do you have the knowledge to say whether or not my understanding is correct?

@mscdex
Copy link
Contributor

mscdex commented Jan 12, 2017

@lance I would say it's a separate issue worth discussing. I am not very familiar overall with the HTTP Keep-Alive implementation in node, so I can't really comment on that.

@lance
Copy link
Member Author

lance commented Jan 24, 2017

@sam-github @mscdex I've incorporated your feedback. Sorry about the delay, I've been traveling quite a bit. I think it's just about there. Have a look when you can. Thanks.

@sam-github
Copy link
Contributor

PTAL @mscdex @lpinca

@lance
Copy link
Member Author

lance commented Jan 25, 2017

Landed in 3b9e8ad

@lance lance closed this Jan 25, 2017
@lance
Copy link
Member Author

lance commented Jan 25, 2017

This has the lts-watch-v6.x tag. Should I go ahead and backport it?

lance added a commit that referenced this pull request Jan 25, 2017
The HTTP Keep-Alive text was inconsistent. These changes follow the
following rules

* When referring to flag used in code, it should always be written using
  backticks and in camel case. E.g. `keepAlive`.
* When referring to the mechanism Keep-Alive functionality as described
  in the HTTP 1.1 RFC, it is written as 'HTTP Keep-Alive', without the
  use of backticks.
* When referring to the request header, it should always use backticks
  and be written as `Connection: keep-alive`.

This commit also includes some changes to how `http.Agent` is
referenced. When `Agent` is used as a reference to an object or
instance, backticks should always be used.

And lastly, the documentation about `Agent` behavior around HTTP
Keep-Alive has been clarified and improved.

Fixes: #10567
PR-URL: #10715

Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@targos
Copy link
Member

targos commented Jan 25, 2017

That shouldn't be necessary. The bot adds the label only when the change applies cleanly.

targos pushed a commit that referenced this pull request Jan 28, 2017
The HTTP Keep-Alive text was inconsistent. These changes follow the
following rules

* When referring to flag used in code, it should always be written using
  backticks and in camel case. E.g. `keepAlive`.
* When referring to the mechanism Keep-Alive functionality as described
  in the HTTP 1.1 RFC, it is written as 'HTTP Keep-Alive', without the
  use of backticks.
* When referring to the request header, it should always use backticks
  and be written as `Connection: keep-alive`.

This commit also includes some changes to how `http.Agent` is
referenced. When `Agent` is used as a reference to an object or
instance, backticks should always be used.

And lastly, the documentation about `Agent` behavior around HTTP
Keep-Alive has been clarified and improved.

Fixes: #10567
PR-URL: #10715

Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
The HTTP Keep-Alive text was inconsistent. These changes follow the
following rules

* When referring to flag used in code, it should always be written using
  backticks and in camel case. E.g. `keepAlive`.
* When referring to the mechanism Keep-Alive functionality as described
  in the HTTP 1.1 RFC, it is written as 'HTTP Keep-Alive', without the
  use of backticks.
* When referring to the request header, it should always use backticks
  and be written as `Connection: keep-alive`.

This commit also includes some changes to how `http.Agent` is
referenced. When `Agent` is used as a reference to an object or
instance, backticks should always be used.

And lastly, the documentation about `Agent` behavior around HTTP
Keep-Alive has been clarified and improved.

Fixes: nodejs#10567
PR-URL: nodejs#10715

Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
The HTTP Keep-Alive text was inconsistent. These changes follow the
following rules

* When referring to flag used in code, it should always be written using
  backticks and in camel case. E.g. `keepAlive`.
* When referring to the mechanism Keep-Alive functionality as described
  in the HTTP 1.1 RFC, it is written as 'HTTP Keep-Alive', without the
  use of backticks.
* When referring to the request header, it should always use backticks
  and be written as `Connection: keep-alive`.

This commit also includes some changes to how `http.Agent` is
referenced. When `Agent` is used as a reference to an object or
instance, backticks should always be used.

And lastly, the documentation about `Agent` behavior around HTTP
Keep-Alive has been clarified and improved.

Fixes: nodejs#10567
PR-URL: nodejs#10715

Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
outstanding requests, so they can be used for future requests without
having to reestablish a TCP connection. Default = `false`
* `keepAliveMsecs` {Integer} When using the `keepAlive` option, specifies
the [initial delay](#net_socket_setkeepalive_enable_initialdelay)
Copy link
Contributor

@Krinkle Krinkle Feb 1, 2017

Choose a reason for hiding this comment

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

This reference is broken. There is no such anchor on the http page. This should link to net.html instead.

Fixed in pull #11108.

Krinkle added a commit to Krinkle/node that referenced this pull request Feb 1, 2017
lpinca pushed a commit that referenced this pull request Feb 4, 2017
Refs: #10715
PR-URL: #11108
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 5, 2017
Refs: nodejs#10715
PR-URL: nodejs#11108
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
Refs: nodejs#10715
PR-URL: nodejs#11108
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
Refs: nodejs#10715
PR-URL: nodejs#11108
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
Refs: #10715
PR-URL: #11108
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
Refs: #10715
PR-URL: #11108
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 8, 2017
The HTTP Keep-Alive text was inconsistent. These changes follow the
following rules

* When referring to flag used in code, it should always be written using
  backticks and in camel case. E.g. `keepAlive`.
* When referring to the mechanism Keep-Alive functionality as described
  in the HTTP 1.1 RFC, it is written as 'HTTP Keep-Alive', without the
  use of backticks.
* When referring to the request header, it should always use backticks
  and be written as `Connection: keep-alive`.

This commit also includes some changes to how `http.Agent` is
referenced. When `Agent` is used as a reference to an object or
instance, backticks should always be used.

And lastly, the documentation about `Agent` behavior around HTTP
Keep-Alive has been clarified and improved.

Fixes: #10567
PR-URL: #10715

Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@jasnell
Copy link
Member

jasnell commented Mar 8, 2017

Landed in v6. This will need a backport PR to land in v4

MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
Refs: #10715
PR-URL: #11108
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
The HTTP Keep-Alive text was inconsistent. These changes follow the
following rules

* When referring to flag used in code, it should always be written using
  backticks and in camel case. E.g. `keepAlive`.
* When referring to the mechanism Keep-Alive functionality as described
  in the HTTP 1.1 RFC, it is written as 'HTTP Keep-Alive', without the
  use of backticks.
* When referring to the request header, it should always use backticks
  and be written as `Connection: keep-alive`.

This commit also includes some changes to how `http.Agent` is
referenced. When `Agent` is used as a reference to an object or
instance, backticks should always be used.

And lastly, the documentation about `Agent` behavior around HTTP
Keep-Alive has been clarified and improved.

Fixes: #10567
PR-URL: #10715

Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
Refs: #10715
PR-URL: #11108
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
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. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

References to an "KeepAlive" flag in the http.Agent constructor, when it is actually "keepAlive"
8 participants