doc: general improvements to net.md copy #7137
Conversation
jasnell
added
doc
doc/api/net.md
Outdated
| The `net` module provides you with an asynchronous network wrapper. It contains | ||
| functions for creating both servers and clients (called streams). You can include | ||
| this module with `require('net');`. | ||
| The `net` module provides an API for implementing network clients and server. |
|
Updated (still have one bit to check on tho) |
|
Hello, there's a (repeated) sentence about the The use of "actual" in this context is ambiguous as there's also a default value. Perhaps these sentences should read something more like "The maximum possible value will be determined by the OS...". (I was about to submit a PR for this but realised it will conflict with the more thorough treatment being proposed here.) |
|
@lovell ... updated! |
|
@nodejs/documentation ... ping |
doc/api/net.md
Outdated
|
|
||
| * `err` {Error} `null` if the operation was successful or `Error` if an | ||
| error occurred. | ||
| * `count` {number} The number of connections |
There was a problem hiding this comment.
After connections should we have a dot ?
|
ping @nodejs/documentation |
1 similar comment
|
ping @nodejs/documentation |
|
|
||
| ```js | ||
| server.on('connection', (socket) => { | ||
| console.log('A connection has been established'); |
There was a problem hiding this comment.
It would be a nice idea to maybe add a comment indicating usage for new readers, explaining what they can do to the socket. It might be obvious though.
|
|
||
| ```js | ||
| server.close((err) => { | ||
| if (err) throw err; |
There was a problem hiding this comment.
I think its very unusual to treat this as a unrecoverable error, perhaps
(err) => {
if (err)
console.log('server wasn't open!')
else
console.log('server is now closed')
}
Generally we wouldn't distinguish between the two, because you wanted the server close, and it is now closed (whether it was open before or not).
| [`child_process.fork()`][]. To poll forks and get current number of active | ||
| connections use asynchronous `server.getConnections` instead. | ||
| connections use the `server.getConnections()` method instead. |
There was a problem hiding this comment.
Isn't it .send() we use to send sockets to child processes over IPC? This makes it sound like .fork() does the fork(2) process duplication, where sockets are also duplicated. Also, if you do spawn with an IPC channel, you can also use .send() to send a socket. I don't think this text was correct.
|
|
||
| * `err` {Error} `null` if the operation was successful or `Error` if an | ||
| error occurred. | ||
| * `count` {number} The number of connections. |
| on the given `handle`. The `handle` object may either be any object with an | ||
| underlying `_handle` member (such as a `net.Socket` or another `net.Server`), | ||
| or an object with a `fd` property whose value is a numeric file descriptor | ||
| (e.g. `{fd: <n>}`). |
There was a problem hiding this comment.
but this will only work if the fd refers to a socket, right? or maybe a pipe? but definitely not a file.
| [`'listening'`][] event. | ||
|
|
||
| The parameter `backlog` behaves the same as in | ||
| [`server.listen(port[, hostname][, backlog][, callback])`][`server.listen(port, host, backlog, callback)`]. | ||
| The `backlog` argument is the maximum size of the queue of pending connections. |
There was a problem hiding this comment.
define pending, perhaps? Its important people not start setting this number to 10,000 because they want that many concurrent TCP connections.
| [`server.listen(port[, hostname][, backlog][, callback])`][`server.listen(port, host, backlog, callback)`]. | ||
| Alternatively, the `path` option can be used to specify a UNIX socket. | ||
| The `server.listen()` method instructs the server to begin accepting connections | ||
| on either the given `host` and `port` or UNIX socket `path`. |
There was a problem hiding this comment.
UNIX -> Local (so that it is true for Windows).
| The `server.listen()` method is asynchronous. When the server has been bound, | ||
| the [`'listening'`][] event will be emitted. | ||
|
|
||
| If a `callback` function is given, it will be added as a listener for the |
There was a problem hiding this comment.
"given", "provided", "specified", "passed" - so many synonyms we use :-(. Not specific to this PR, but do you have a favorite that we should push to use across the API docs?
| @@ -183,15 +283,18 @@ added: v0.1.90 | |||
| * `backlog` {Number} | |||
| * `callback` {Function} | |||
|
|
|||
| Start a local socket server listening for connections on the given `path`. | |||
| The `server.listen()` method instructs the server to begin accepting connections | |||
There was a problem hiding this comment.
This is a (pre-existing) mess, IMO.
All the docs here also need to be in the API above, for when options is passed with a .path, but that would cause unreadable duplication, but ATM they have a non overlapping set of information, which is not good, either.
IMO, I would make the .options variant either specifically reference/link to the docs for the path-based .listen() to describe its .path option, or else move almost all the docs here (but not the example) up into the options-taking version of the API, and then document this path-taking version as a variant of the options-taking (which is how its actually implemented). The latter has the advantage of keeping everything centralized, at the price of making the one API really big.
Right now, its neither one approach, or the other.
| `hostname` is omitted, the server will accept connections on any IPv6 address | ||
| (`::`) when IPv6 is available, or any IPv4 address (`0.0.0.0`) otherwise. Use a | ||
| port value of `0` to have the operating system assign an available port. | ||
| The `server.listen()` method instructs the server to begin accepting connections |
There was a problem hiding this comment.
here as well it should reference/link to the options-based variant
duplicating the text is not only a problem from a maintainability point of view, its also a problem from a readers point of view.
ATM, without reading the code or doing a side-by-side and line-by-line textual comparison of the options-based and port+host based API documentation, I cannot know that the handling of (for example) hostname defaults is identical for the two APIs (because they are in fact the same API). The docs should make clear that they are alternative ways of providing the options, but the behaviour associated with the options is identical, no matter how they are passed (positional arg or options object).
| If a `callback` function is given, it will be added as a listener for the | ||
| [`'listening'`][] event. | ||
|
|
||
| An `EADDRINUSE` error will occur if another server is already bound to the |
There was a problem hiding this comment.
This is a good example of text that is present here, but not in the options-based, even though it applies equally, of course.
| The `server.maxConnections` property can be set to specify the maximum number | ||
| of connections the server will accept. Once the current number of connections | ||
| reaches this threshold, all additional connection requests will be rejected. | ||
| The default is `undefined`. |
There was a problem hiding this comment.
", meaning that no maximum is enforced by Node. Note that operating system limits on open sockets will still apply and be enforced by the system."
There was a problem hiding this comment.
not sure if it can be deleted or re-assigned, or what happens when set to a value lower than current number of connections, or if it has to be set before .listen() is called on the server.
| It is not recommended to use this option once a socket has been sent to a child | ||
| with [`child_process.fork()`][]. | ||
| *Note*: Setting this property *after* a socket has been sent to a child | ||
| using [`child_process.fork()`][] is *not recommended*. |
There was a problem hiding this comment.
recommended or not... what will the behaviour be?
| the server is `ref`d calling `ref` again will have no effect. | ||
| Calling the `server.ref()` method will cause the Node.js event loop to continue | ||
| so long as the `server` is not closed. Multiple calls to `server.ref()` will | ||
| have no effect. |
There was a problem hiding this comment.
It looks like you are trying to get out of the double-negative situation of previous docs, but the new text implies that ref() must be called, when sockets are refed by default.
I feel quite strongly that the docs for the event loop and aliveness should be in one place, and this should just link, as should the many other resources.
At least, the text should be consistent between the various handle-based resources, like https://nodejs.org/api/timers.html#timers_timeout_ref
|
|
||
| Returns `server`. | ||
| Returns a reference to `server`. |
There was a problem hiding this comment.
what are you trying to clarify here? all objects are passed by ref in javascript. This way of stating that is not typical, cf.
The listener callback is called with the
net.Socketobject for the connection
which doesn't say "called with a referenct to the net.Socket object"
| open" state. Defaults to `false`. | ||
| * `readable` {boolean} If `fd` is specified, setting `readable` to `true` | ||
| allows data from the socket to be read. Defaults to `false`. | ||
| * `writable` {boolean} If `fd` is specified, setting `writabl` to `true` |
| * `readable` {boolean} If `fd` is specified, setting `readable` to `true` | ||
| allows data from the socket to be read. Defaults to `false`. | ||
| * `writable` {boolean} If `fd` is specified, setting `writabl` to `true` | ||
| allows data to be written to the socket. Defaults to `false`. |
There was a problem hiding this comment.
To confirm: default when using fd is it is neither readable nor writable?
|
|
||
| ### Event: 'close' | ||
| <!-- YAML | ||
| added: v0.1.90 | ||
| --> | ||
|
|
||
| * `had_error` {Boolean} `true` if the socket had a transmission error. | ||
| The `'close'` event is emitted once the socket is closed and can no longer be | ||
| used to send or receive data. |
There was a problem hiding this comment.
if half open is allowed, doesn't it just mean no more data will be read, but its still writable?
| used to send or receive data. | ||
|
|
||
| The listener callback is called with a single boolean `had_error` argument. | ||
| If `true`, then the socket was closed due to a transmission error. |
There was a problem hiding this comment.
In this case, an error event would have been emitted before the close. So, error events are always followed by close events with the had_error agument true.
| The `'connect'` event is emitted when a socket connection is successfully | ||
| established. See [`net.connect()`][]. | ||
|
|
||
| The listener callback is called without passing any arguments. |
There was a problem hiding this comment.
I think the word "passing" could be deleted
|
|
||
| Note that the __data will be lost__ if there is no listener when a `Socket` | ||
| *Note*: __Data will be lost__ if there is no listener when a `net.Socket` |
There was a problem hiding this comment.
yeah, but don't they start paused, so they won't emit the data event until someone listens on it, or calls resume()?
| write queue. However, by setting `allowHalfOpen` to `true`, the socket will not | ||
| automatically end its side of the socket allowing additional data to be written. | ||
|
|
||
| The listener callback is called without passing any arguments. |
There was a problem hiding this comment.
passing could be deleted
| Emitted after resolving the hostname but before connecting. | ||
| Not applicable to UNIX sockets. | ||
| The `'lookup'` event is emitted after the `net.Socket` has resolved the | ||
| `hostname` given when connection, but *before* the connection is established. |
| Not applicable to UNIX sockets. | ||
| The `'lookup'` event is emitted after the `net.Socket` has resolved the | ||
| `hostname` given when connection, but *before* the connection is established. | ||
| This event is not applicable when using UNIX sockets. |
There was a problem hiding this comment.
is not emitted when using local sockets, because no address resolution is required.
|
|
||
| * `port` {number} | ||
| * `family` {String} Either `'IPv4'` or `'IPv6'` | ||
| * `address` {String} The IPv4 or IPv6 address |
There was a problem hiding this comment.
unless its unbound, in which case the object has no properties. and wouldn't it be a path if it was a local socket?
| The read-only `socket.bufferSize` property specifies the current amount of data | ||
| currently pending in the `net.Socket` instances write buffer. | ||
|
|
||
| The `net.Socket` class has been designed such that calls to `socket.write()` |
There was a problem hiding this comment.
seems to be describing things that are better described in the streams docs, this version will miss the warnings in #10631
| written, but the buffer may contain strings, and the strings are lazily | ||
| encoded, so the exact number of bytes is not known.) | ||
| The value of `socket.bufferSize` represents the amount of data (in bytes) | ||
| pending in the queue. |
There was a problem hiding this comment.
represents, or is? Its not a scaled, I don't think.
| * `port` {number} Port the client should connect to | ||
| * `host` {String} Host the client should connect to | ||
| Defaults to `'localhost'` | ||
| * `localAddress` {String} Local interface to bind to for network connections |
| * `host` {String} Host the client should connect to | ||
| Defaults to `'localhost'` | ||
| * `localAddress` {String} Local interface to bind to for network connections | ||
| * `localPort` {number} Local port to bind to for network connections |
| Enable/disable keep-alive functionality, and optionally set the initial | ||
| delay before the first keepalive probe is sent on an idle socket. | ||
| `enable` defaults to `false`. | ||
| * `enable` {boolean} `true` to enable keep-alive, `false` to disable. Defaults |
There was a problem hiding this comment.
should make it clear these are TCP level keep alives, maybe even explicitly mention SOL_KEEPALIVE so people can google for it, and perhaps warn that it should not be used unless the limitations of the TCP keep alive are understood
| `true` | ||
|
|
||
| The `socket.setNoDelay()` method enables or disables Nagle's algorithm. By | ||
| default TCP connections use Nagle's algorithm and buffer data before sending |
There was a problem hiding this comment.
algorithm to concatenate the buffers from a series of small writes into one larger packet before sending it over the network. The default is designed for interactive shell type applications, .setNoDelay(true) should be used for most TCP based protocols.
| Returns a reference to `socket`. | ||
|
|
||
| *Note*: Disabling Nagle's algorithm is primarily useful when working with | ||
| protocols that use many small payloads as it reduces or eliminates the |
There was a problem hiding this comment.
doesn't it reduce the sending of many small packets, which cause a high protocol overhead to transported data ratio?
|
|
||
| Returns `true` if the entire data was flushed successfully to the kernel | ||
| buffer. Returns `false` if all or part of the data was queued in user memory. | ||
| [`'drain'`][] will be emitted when the buffer is again free. | ||
| The [`'drain'`][] event will be emitted when the buffer is again free. |
There was a problem hiding this comment.
"when the queue has been drained by sending all the data on the socket"?
| pauseOnConnect: false | ||
| } | ||
| ``` | ||
| * `options` {Object} |
There was a problem hiding this comment.
I've read these docs above... can't we link, so that its crystal clear the behaviour is identical?
|
This was a great start to reworking the net docs! You don't have the time to keep it going? |
|
At the moment no. Happy to hand it over! :) |
|
I'll add it to my long and growing list of node doc problems, but no promises. |
Checklist
Affected core subsystem(s)
doc (net)
Description of change
General improvements to net.md copy
@nodejs/documentation