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

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.

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.

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.

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

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

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

• Always send HTTP/1.0Connection: keep-alive headers. See: https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L276-L279
• Destroy socket connections when there are no more pending requests for a given host/port. See: https://github.com/nodejs/node/blob/master/lib/_http_agent.js#L47-L81
• Not implement HTTP/1.1 persistent connections, since sockets are destroyed by default (see above)
• Not pool socket connections, unless there are multiple, simultaneous requests to the same host/port (see above)
• To use connection pooling, you must specify { keepAlive: true } as a constructor option in a new Agent
• When using connection pooling, HTTP/1.0 connection persistence is enabled using the Connection: keep-alive header. See above re: always sending Connection: keep-alive
• When using connection pooling, and therefore also HTTP/1.0 connection persistence via Connection: keep-alive headers (because you can't have one without the other in this implementation), TCP SO_KEEPALIVE is used via lib_uv's uv_tcp_keepalive function. See: https://github.com/nodejs/node/blob/master/lib/_http_agent.js#L73, https://github.com/nodejs/node/blob/master/lib/net.js#L368, and https://github.com/nodejs/node/blob/master/src/tcp_wrap.cc#L161
• None of this matters at all if you provide a createConnection function to the http.request() function.

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

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

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

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

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

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

PTAL @mscdex @lpinca

Landed in 3b9e8ad

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

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

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