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: improve docs for net.connect, net.createConnection, net.Socket #11700

Closed
wants to merge 7 commits into from

Conversation

@joyeecheung
Copy link
Member

commented Mar 5, 2017

This is a follow up of #11636.

  • Nest the overloaded arguments
  • Added type annotations
  • alias net.connect to net.createConnection (now in the code they are ===)
  • Add links to man pages for reference
  • General improvements to wording and explanation
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc, net

@joyeecheung joyeecheung force-pushed the joyeecheung:net-socket-doc branch Mar 5, 2017

doc/api/net.md Outdated
user and used as a client (with [`connect()`][]) or they can be created by Node.js
and passed to the user through the `'connection'` event of a server.
This class is an abstraction of a TCP socket, a UNIX domain socket, or
a Windows named pipe. A `net.Socket` is also a [duplex stream][], so it

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 6, 2017

Member

consider: a abstraction of a TCP or local streaming socket (local uses named pipes on Windows, and UNIX domain sockets otherwise) ... this long list of underlying technologies seemed random to you at first, I think it will confuse others, and it would be good to emphasize the OS mechanism being abstracted is "local streaming IPC", it just happens to have different names on UNIX and Windows (the Windows O/S devs weren't about to call a core IPC mechanism "unix" I guess ;-).

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Mar 7, 2017

Author Member

I have considered using the term IPC but remembered people are using net abstractions over Windows named pipes for RPC as well? (I think I've seem issues about access permissions before, couldn't find the link).

So maybe: a abstraction of a TCP or streaming socket (uses named pipes on Windows, and UNIX domain sockets otherwise) (loses the local part)?

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Mar 7, 2017

Author Member

Er, wait, I think IPC still works because technically the name IPC doesn't imply it's local, it can be remote. s/streaming socket/streaming IPC/?

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 7, 2017

Member

I've never seen anything that suggests that windows named pipes can be used between machines. And IPC doesn't have an exact definition, its like the word "process" itself, but it does usually imply same-host. Anyhow, I'm ok with any reasonable phrase, that we define and then use instead of writing out "named pipes on Windows and UNIX domain sockets on non-Windows" over and over.... @piscisaureus can named pipes be used between hosts?

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Mar 7, 2017

Author Member

FWIW, the MSDN says:

Named pipes can be used to provide communication between processes on the same computer or between processes on different computers across a network. If the server service is running, all named pipes are accessible remotely. If you intend to use a named pipe locally only, deny access to NT AUTHORITY\NETWORK or switch to local RPC.

I am not sure if Node.js actually supports this, but looks like the remote access is configured outside the application?
(er, nevermind, just saw your comment below this :D)

doc/api/net.md Outdated
a Windows named pipe. A `net.Socket` is also a [duplex stream][], so it
can be both readable and writable, and it is also a [`EventEmitter`][].

A `net.Socket` can be created by the user and used as an abstraction

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 6, 2017

Member

"used as an abstraction of the client" is a bit convoluted in my opinion, consider: "used directly to connect to a server". Almost all programming objects are in some sense "abstractions" (a url.URL is an abstraction of a "URL", a js-String is an abstraction of a "sequence of characters", etc., but its not how we usually describe them).

doc/api/net.md Outdated
of the client. For example, it is returned by [`net.createConnection()`][],
so the user can use it to interact with the server.

It can also be be created by Node.js and passed to the user as an abstraction

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 6, 2017

Member

consider: "passed to the user when a connection is received"

doc/api/net.md Outdated
`net.Socket` instances are [`EventEmitter`][] with the following events:
* `options` {Object} Available options are:
* `fd`: {number | null} Defaults to `null`. If specified, wrap around an
existing socket with the given file descriptor.

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 6, 2017

Member

otherwise ... ? If unspecified, the fd/socket will be created should be stated.

doc/api/net.md Outdated
existing socket with the given file descriptor.
* `allowHalfOpen` {boolean} Default to `false`. Indicates whether
half-opened TCP connections are allowed. See [`net.createServer()`][]
and [`'end'`][] event for details.

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 6, 2017

Member

and the end event for details <--- need a "the"

doc/api/net.md Outdated
- `hints`: [`dns.lookup()` hints][]. Defaults to `0`.
* `port` {number} Required. Port the socket should connect to.
* `host` {string} Host the socket should connect to. Defaults to `'localhost'`.
* `localAddress` {string} Optional. Local address the socket should send

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 6, 2017

Member

"should connect from" (you don't really "send" connections)

doc/api/net.md Outdated
* `host` {string} Host the socket should connect to. Defaults to `'localhost'`.
* `localAddress` {string} Optional. Local address the socket should send
connections from.
* `localPort` {number} Optional. Local port the socket should send connections

This comment has been minimized.

Copy link
@sam-github
doc/api/net.md Outdated

`net.Socket` instances are [`EventEmitter`][] with the following events:
* `options` {Object} Available options are:
* `fd`: {number | null} Defaults to `null`. If specified, wrap around an

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 6, 2017

Member

general comment: there is a fairly random mix of style wrt. where the default is mentioned. sometines the default is first thing mentioned (here), sometimes it is the last thing (net.connect). I think it makes more sense to describe what an option does before describing its default.

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Mar 7, 2017

Author Member

+1 to describe the meaning before describing default. Also some docs use "Defaults to", some use "Default to". Not sure if we need to use present tense in the docs if a sentence starts this way? (This is a bit inconsistent in almost every API docs I've ever seen, not just Node.js..)

(random idea: it would be great if we have some structural way to describe the default, and let the doc tools generate something with different styles).

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 7, 2017

Member

"Default to X" is ungrammatical, IMO. "Defaults to X" and "Default is X" both sound OK to me. Tooling would be good, too! Lots of work, though.

doc/api/net.md Outdated

Normally this method is not needed, as `net.createConnection` opens the
socket. Use this only if you are implementing a custom Socket.
* `path` {string} Required. Path the client should connect to.

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 6, 2017

Member

I think "Required." is unnecessary, its clearly not optional in the syntax above. There would be an awful lot of "Required." in our docs if we put that as the first word sentence for every mandatory positional parameter, which we don't.

doc/api/net.md Outdated

## net.createConnection(options[, connectListener])
This function is asynchronous. When the connection is established, a

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 6, 2017

Member

This function is asynchronous.

I think this is misleading. The connection object is returned synchronously, but the establishment of the connection is asynchronous. Better would be something like:

Creates a socket object and initiates a connection. When the connection is established....

@joyeecheung joyeecheung force-pushed the joyeecheung:net-socket-doc branch Mar 7, 2017

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2017

@sam-github Thanks for the review! Addressed most comments, apart from #11700 (comment).

Added a section about IPC support and how to specify the paths (mostly moved from server.listen docs), and made other relevant APIs link to it.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2017

@sam-github Removed the man pages references...PTAL, thanks

@sam-github
Copy link
Member

left a comment

I like the way you have brought the IPC section out to a seperate section, looks great.

doc/api/net.md Outdated
### Identifying paths for IPC connections

[`net.connect()`][], [`net.createConnection()`][], [`server.listen()`][] and
[`socket.connect()`][] takes a `path` parameter to identify IPC endpoints.

This comment has been minimized.

Copy link
@sam-github
doc/api/net.md Outdated
[`net.connect()`][], [`net.createConnection()`][], [`server.listen()`][] and
[`socket.connect()`][] takes a `path` parameter to identify IPC endpoints.

On UNIX, the local domain is usually known as the UNIX domain. The path is a

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 8, 2017

Member

usually -> also

doc/api/net.md Outdated
*Note*: Unix named pipes (FIFOs) are not supported.

On Windows, the local domain is implemented using a named pipe. The path *must*
refer to an entry in `\\?\pipe\` or `\\.\pipe\`. Any characters are permitted,

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 8, 2017

Member

I thought if you listen on a path like 'path' it becomes \\?\pipe\path, not true?

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Mar 9, 2017

Author Member

There will be an EACCESS if the path is not prepended with that..see nodejs/node-v0.x-archive#2797 (comment)

doc/api/net.md Outdated
socket (NOTE: Works only when `fd` is passed).
About `allowHalfOpen`, refer to [`net.createServer()`][] and [`'end'`][] event.
* `options` {Object} Available options are:
* `fd`: {number | null} If specified, wrap around an existing socket with

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 8, 2017

Member

{number} is sufficient, we don't call out undefined/null as independent types for unspecified options in our docs, not anywhere I can recall seeing. You don't do on the 3 other options that default to null just below, for example.

doc/api/net.md Outdated

It can also be be created by Node.js and passed to the user when a connection
is received. For example, it is passed to the listeners of a
[`'connection'`][] event fired on a [`net.Server`][], so the user can use

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 8, 2017

Member

"fired" -> "emitted"

doc/api/net.md Outdated
[half-closed]: https://tools.ietf.org/html/rfc1122#section-4.2.2.13
[IPC]: #net_ipc_support

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 8, 2017

Member

out of order, use default/ASCII/"C" locale sorting

doc/api/net.md Outdated

```js
const net = require('net');
```

## IPC Support

The `net` module supports IPC with named pipes on Windows, and domain sockets

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 8, 2017

Member

"UNIX domain sockets", they are never called "domain sockets"! http://man7.org/linux/man-pages/man7/unix.7.html

doc/api/net.md Outdated
on file creation. It will be visible in the filesystem, and will *persist until
unlinked*.

*Note*: Unix named pipes (FIFOs) are not supported.

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 8, 2017

Member

This makes it sound like its a missing feature, when actually its an entirely diffferent IPC pattern that doesn't map to duplex streams, or to client/server communication patterns. Also, I expect that they are supported by node... using the fs API. If you are going to mention them here, I think you should mention something like "FIFOs on Unix are sometimes called "named pipes", but despite the similarity in name to Windows named pipes, they function completely differently, and aren't supported or relevant to the net API.". I think this is getting off track, though. The only people who would read this section and think "wait, what about UNIX named pipes?" are people who already know that there are UNIX named pipes... and those people already know that a FIFO is not representable as a bi-directional stream (IMO). I'm not wondering about unix pipes, socketpairs, or message queues when I read the net docs, either.

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Mar 9, 2017

Author Member

Yes it does look stranger if pulled out to here, where the context of net being about bidirectional streams is clearer. This can be removed.

@sam-github

This comment has been minimized.

Copy link
Member

commented Mar 9, 2017

or maybe I should have waited before approving, but when the nits you thumbed up above are addressed, this LGTM

joyeecheung added 4 commits Mar 7, 2017
doc: improve socket.connect description
* Nest the overloaded signatures
* Add references to system call manuals
* Improve links
doc: improve net.createConnection description
* Nest the overloaded signatures
* Improve links

@joyeecheung joyeecheung force-pushed the joyeecheung:net-socket-doc branch Mar 9, 2017

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2017

Addressed comments and rebased.

CI: https://ci.nodejs.org/job/node-test-pull-request/6779/

doc: improve net.Socket class description
* Add type annotations to the options
* Improve the descriptions of net.Socket class

@joyeecheung joyeecheung force-pushed the joyeecheung:net-socket-doc branch 2 times, most recently to 090607c Mar 9, 2017

@jasnell
Copy link
Member

left a comment

LGTM with a nit


### socket.connecting
<!-- YAML
added: v6.1.0
-->

If `true` - [`socket.connect(options[, connectListener])`][`socket.connect(options, connectListener)`] was called and
If `true` - [`socket.connect(options[, connectListener])`][`socket.connect(options)`] was called and

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 10, 2017

Member

Please wrap at 80 chars :-)

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2017

@jasnell Fixed 80-character wrap, thanks.

(Aside: the wording of socket.connecting can be improved...probably in another PR)

Going to land this tomorrow.

@joyeecheung joyeecheung force-pushed the joyeecheung:net-socket-doc branch 2 times, most recently to ce0c668 Mar 10, 2017

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2017

Landed in df97727, thanks!

joyeecheung added a commit that referenced this pull request Mar 11, 2017
doc: improve net.md on sockets and connections
* Improve description of socket.connect, net.connect,
  net.createConnection
* Nest the overloaded signatures
* Alias net.connect to net.createConnection
* Add type annotations to the options
* Make a separate section to explain IPC support and
  how to specify the path parameter.
* General improvements to wording and explanation

PR-URL: #11700
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@italoacasas

This comment has been minimized.

Copy link
Member

commented Mar 13, 2017

@joyeecheung this is not landing clearly in v7.x, can backport?

jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017
doc: improve net.md on sockets and connections
* Improve description of socket.connect, net.connect,
  net.createConnection
* Nest the overloaded signatures
* Alias net.connect to net.createConnection
* Add type annotations to the options
* Make a separate section to explain IPC support and
  how to specify the path parameter.
* General improvements to wording and explanation

PR-URL: nodejs#11700
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@jasnell jasnell referenced this pull request Apr 4, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 17, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@joyeecheung joyeecheung self-assigned this Oct 17, 2017

@joyeecheung joyeecheung removed their assignment Nov 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.