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: general improvements to net.md copy #7137

Closed
wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jun 3, 2016

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

doc (net)

Description of change

General improvements to net.md copy

@nodejs/documentation

@jasnell jasnell added doc Issues and PRs related to the documentations. net Issues and PRs related to the net subsystem. labels Jun 3, 2016
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.
Copy link
Member

Choose a reason for hiding this comment

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

typo: servers

@jasnell
Copy link
Member Author

jasnell commented Jun 6, 2016

Updated (still have one bit to check on tho)

@lovell
Copy link
Contributor

lovell commented Jun 8, 2016

Hello, there's a (repeated) sentence about the backlog parameter of listen() that states "The actual length will be determined by the OS...".

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

@jasnell
Copy link
Member Author

jasnell commented Jun 9, 2016

@lovell ... updated!

@jasnell
Copy link
Member Author

jasnell commented Jun 21, 2016

@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
Copy link
Contributor

Choose a reason for hiding this comment

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

After connections should we have a dot ?

@jasnell
Copy link
Member Author

jasnell commented Jun 28, 2016

ping @nodejs/documentation

1 similar comment
@jasnell
Copy link
Member Author

jasnell commented Jul 3, 2016

ping @nodejs/documentation


```js
server.on('connection', (socket) => {
console.log('A connection has been established');
Copy link
Member

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

"if err is null"

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>}`).
Copy link
Contributor

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

"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
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

", 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."

Copy link
Contributor

Choose a reason for hiding this comment

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

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*.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.
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 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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.Socket object 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`
Copy link
Contributor

Choose a reason for hiding this comment

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

"writabl" <-- spelling

* `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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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`
Copy link
Contributor

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

when "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.
This event is not applicable when using UNIX sockets.
Copy link
Contributor

Choose a reason for hiding this comment

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

is not emitted when using local sockets, because no address resolution is required.

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.

Nice work, James, much improved.


* `port` {number}
* `family` {String} Either `'IPv4'` or `'IPv6'`
* `address` {String} The IPv4 or IPv6 address
Copy link
Contributor

Choose a reason for hiding this comment

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

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()`
Copy link
Contributor

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

default?

* `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
Copy link
Contributor

Choose a reason for hiding this comment

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

default?

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

"when the queue has been drained by sending all the data on the socket"?

pauseOnConnect: false
}
```
* `options` {Object}
Copy link
Contributor

Choose a reason for hiding this comment

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

I've read these docs above... can't we link, so that its crystal clear the behaviour is identical?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Feb 9, 2017
@jasnell jasnell closed this Feb 15, 2017
@sam-github
Copy link
Contributor

This was a great start to reworking the net docs! You don't have the time to keep it going?

@jasnell
Copy link
Member Author

jasnell commented Feb 16, 2017

At the moment no. Happy to hand it over! :)

@sam-github
Copy link
Contributor

sam-github commented Feb 16, 2017

I'll add it to my long and growing list of node doc problems, but no promises.

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. net Issues and PRs related to the net subsystem. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants