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

fixed TLSSocket documentation error from Issue #3963. #14062

Closed
wants to merge 13 commits into from

Conversation

VerteDinde
Copy link
Contributor

#Checklist
Affected core subsystem(s)

This PR affects the documentation for TLSSocket and new.Socket, and references Issue #3963. See line 879 - 902 in doc/api/tls.md. Let me know if I should make any changes! #3963

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem. labels Jul 3, 2017
@tniessen
Copy link
Member

tniessen commented Jul 3, 2017

Thank you for your contribution! To start off, head over to our style guide. For example, you should

Generally avoid personal pronouns in reference documentation ("I", "you", "we").

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Regardless of the technical contents, here are some basic suggestions for improval 😃

doc/api/tls.md Outdated
@@ -876,6 +876,30 @@ socket.on('end', () => {
});
```

Note: When using an instance of `net.Socket`, you start the `net.Socket`. You can use `net.Socket` to upgrade an existing socket you don't wrap the `net.Socket` in a `TLSSocket`, then the connect works as expected.
Copy link
Member

Choose a reason for hiding this comment

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

Avoid using you (see my comment above).

Copy link
Member

Choose a reason for hiding this comment

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

There is a grammar mistake in the second sentence: "You can use net.Socket to upgrade an existing socket ??? you don't wrap the..."

doc/api/tls.md Outdated
See the example below for usage of upgrading an existing socket with `net.Socket`:

```js
var Socket = require('net').Socket;
Copy link
Member

Choose a reason for hiding this comment

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

Use const instead of var where applicable.

doc/api/tls.md Outdated
var secureSock = tls.connect({ socket: s }, function() {
console.log("The tls socket connected. Yay!");
});
sock.connect({port: 6697, host: "irc.freenode.net"});
Copy link
Member

Choose a reason for hiding this comment

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

Style nits: Use 'foo' instead of "foo" and put spaces around the properties like { foo: 'bar' }.

doc/api/tls.md Outdated
sock.connect({port: 6697, host: "irc.freenode.net"});
```

That's typically how you upgrade an existing socket. However, if you're using TLS from the start, then just use tls.connect() to upgrade a socket:
Copy link
Member

Choose a reason for hiding this comment

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

Avoid using informal omissions: "you're" → "you are".

Copy link
Member

Choose a reason for hiding this comment

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

Put tls.connect() into backticks to mark it as code.

doc/api/tls.md Outdated
var tls = require('tls');
var secureSock = tls.connect({port: 6697, host: "irc.freenode.net"}, function() {
console.log("The tls socket connected. Yay!");
secureSock.write(...);
Copy link
Member

Choose a reason for hiding this comment

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

We like to keep our examples as close to runnable code as possible. This will cause a parser error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this parse error is fixed now, but (I apologize) I am not terribly familiar with tls.connect and was using the recommended fix referenced in the issue. If there is a better example, happy to use it instead.

doc/api/tls.md Outdated

```js
var tls = require('tls');
var secureSock = tls.connect({port: 6697, host: "irc.freenode.net"}, function() {
Copy link
Member

Choose a reason for hiding this comment

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

Avoid using function() { ... } where arrow functions can be used: () => { ... }

@VerteDinde
Copy link
Contributor Author

Thank you for your time and all of the thoughtful comments, @tniessen! Sorry for all my errors - I'm going through your comments and fixing everything now, will have an updated push shortly. 😄

@VerteDinde
Copy link
Contributor Author

PR updated to address all comments. 😃 Will be happy to make any additional changes.

doc/api/tls.md Outdated
@@ -876,6 +876,27 @@ socket.on('end', () => {
});
```

When using an instance of `net.Socket`, use `net.Socket` to upgrade an existing socket. Do not wrap the `net.Socket` in a `TLSSocket`. See the example below for usage of upgrading an existing socket:
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!
Our doc style guide recommends to word-wrap lines at 80 characters)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do! Making changes now, thank you @vsemozhetbyt

doc/api/tls.md Outdated
When using an instance of `net.Socket`, use `net.Socket` to upgrade an existing socket. Do not wrap the `net.Socket` in a `TLSSocket`. See the example below for usage of upgrading an existing socket:

```js
const Socket = require('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.

Most of our examples use destructuring, so this can be:

const { Socket } = require('net');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Changed to the destructured format.

doc/api/tls.md Outdated
@@ -876,6 +876,31 @@ socket.on('end', () => {
});
```

When using an instance of `net.Socket`, use `net.Socket` to upgrade an existing socket.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, this is still 88 chars) But it is a small nit and can be fixed later on landing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah dang, I can't count! Sorry, now fixed - cuts off before 80.

doc/api/tls.md Outdated
sock.connect({ port: 6697, host: 'irc.freenode.net' });
```

If using TLS as the initial default rather than 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.

`net.Socket`)

doc/api/tls.md Outdated
const secureSock = tls.connect({ socket: s }, () => {
console.log('The tls socket connected.');
});
sock.connect({ port: 6697, host: 'irc.freenode.net' });
Copy link
Member

Choose a reason for hiding this comment

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

I think this works, but our docs say

Usually, a socket is already connected when passed to tls.connect()

It might be a bit counter-intuitive to call tls.connect() before socket.connect(). I don't use the TLS module much, is there any reason for this order?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question - I thought that seemed off as well, but worked when I tested it. Let me test it by calling sock.connect first and see; it does seem redundant to call sock.connect after tls.connect.

Copy link
Contributor Author

@VerteDinde VerteDinde Jul 3, 2017

Choose a reason for hiding this comment

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

I replaced { socket: s } with the port and host information, and removed sock.connect entirely. It appears to be passing, and now the sample code is more in line with the docs' recommendations.

doc/api/tls.md Outdated
@@ -876,6 +876,27 @@ socket.on('end', () => {
});
```

When using an instance of `net.Socket`, use `net.Socket` to upgrade an existing socket. Do not wrap the `net.Socket` in a `TLSSocket`. See the example below for usage of upgrading an existing socket:
Copy link
Member

Choose a reason for hiding this comment

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

When using an instance of net.Socket, use net.Socket to upgrade an existing socket.

I think this sentence is misleading. Users still need to use tls.connect(). Perhaps something like

To upgrade an existing instance of net.Socket to a tls.TLSSocket, pass it to tls.connect() as the socket option:

(code here)

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's much more clear - the original issue had a user trying to do this:

const tlsSocket = new TlsSocket(new NetSocket());

And I think your wording will keep them from doing that much more clearly. Will update now!

@vsemozhetbyt
Copy link
Contributor

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Formal part LGTM

doc/api/tls.md Outdated
const { Socket } = require('net');
const tls = require('tls');
const sock = new Socket();
const secureSock = tls.connect({ port: 6697, host: 'irc.freenode.net' }, () => {
Copy link
Member

Choose a reason for hiding this comment

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

Are we legally allowed and/or encouraged to use a third party domain here? @addaleax

Copy link
Member

Choose a reason for hiding this comment

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

In the documentation? I wouldn’t worry about that, although example.org:443 might be a somewhat more natural choice?

Copy link
Member

Choose a reason for hiding this comment

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

example.org exists for that sole purpose, but users won't be able to connect to it.

Copy link
Member

Choose a reason for hiding this comment

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

https://example.org:443/ seems to work fine for me?

Copy link
Member

Choose a reason for hiding this comment

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

Oh okay, I thought example.com rejects all IP traffic, seems like HTTP and HTTPS work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! Added 'https://example.org:443/' instead of the freenode url.

doc/api/tls.md Outdated
@@ -876,6 +876,30 @@ socket.on('end', () => {
});
```

Passing in `net.Socket` will start a new instance.
To upgrade an existing instance of `net.Socket` to a
Copy link
Member

Choose a reason for hiding this comment

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

Please try to avoid trailing whitespace characters. Git can help you with that, see this.

(This is not critical, our team will usually fix this while landing the PR, but it is still good practice to take care of that! 😃 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry, and thank you for pointing it out! 😄 Thanks so much for all of your help, this was my first open source PR and you were wonderful to work with.

doc/api/tls.md Outdated
```js
const { Socket } = require('net');
const tls = require('tls');
const sock = new Socket();
Copy link
Member

Choose a reason for hiding this comment

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

Where is this variable used?

Copy link
Member

Choose a reason for hiding this comment

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

Uhm something went wrong here... The original code (see first commit) looked like tls.connect({ socket: sock }). @VerteDinde You probably don't want to use tls.connect({ port: 6697, host: 'irc.freenode.net' }) in both cases, right? This won't upgrade an existing socket, it establishes a new connection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right! Just updated to reflect that variable and changed the irc.freenode.net to https://example.org:443/.

doc/api/tls.md Outdated
@@ -895,7 +895,7 @@ use only `tls.connect()` to upgrade the socket:

```js
const tls = require('tls');
const secureSock = tls.connect({ port: 6697, host: 'irc.freenode.net' }, () => {
const secureSock = tls.connect({ port: 6697, host: 'https://example.org:443/' }, () => {
Copy link
Member

Choose a reason for hiding this comment

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

I am sorry if we were too vague about this, but this is not how TLS sockets work. You need to specify example.org as the host and 443 as the port instead of 6697. https://example.org:443/ is a URL, @addaleax just used it to point out that browsers can connect to port 443. Sockets are below HTTP level, HTTP(s) is built on top of (TLS) sockets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, sorry - this is now fixed. Thanks so much for your patience, @tniessen 😝

Copy link
Member

@TimothyGu TimothyGu 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! I'd actually move the bits you added, so that is comes before the "echo server" example. Usually for API documentation we want to first explain what the API does, before providing a full working example like the "echo server".

doc/api/tls.md Outdated
const tls = require('tls');
const sock = new Socket();
const secureSock = tls.connect({ socket: sock }, () => {
console.log('The tls socket connected.');
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer 'The TLS socket has been connected.'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. 😄 Altered!

doc/api/tls.md Outdated
```

If using TLS as the initial default rather than `net.Socket`,
use only `tls.connect()` to upgrade the socket:
Copy link
Member

Choose a reason for hiding this comment

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

I think "upgrade" is a bit of a misnomer as there is no initial socket to be upgraded. I'd go with just:

If no socket is provided, this function will create a new TLS socket.

as opposed to

If a net.Socket is provided, this function will upgrade that TCP socket to a TLS one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhhh this is actually really clarifying - I was wondering what the distinction was with net.Socket. Have clarified by removing my old sentence, and adding yours. Thanks!

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Left a comment. You can treat it as optional.

doc/api/tls.md Outdated
const { Socket } = require('net');
const tls = require('tls');
const sock = new Socket();
const secureSock = tls.connect({ socket: sock }, () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you rename sock to socket, you can drop the : sock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks man! Destructured.

@tniessen tniessen self-assigned this Jul 4, 2017
doc/api/tls.md Outdated
@@ -876,6 +876,30 @@ socket.on('end', () => {
});
```

Passing in `net.Socket` will start a new instance.
Copy link
Member

Choose a reason for hiding this comment

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

Is this line necessary? I am not entirely sure I understand what it is supposed to say, apart from the same thing the next sentence says.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. In the original issue, the user was attempting to do this because he didn't understand that if you pass in a net.Socket, you start the net.Socket.:

const tlsSocket = new TlsSocket(new NetSocket());

I just wanted to make sure that that was clearly stated; but if it's clear in the subsequent lines, I'll just take it out 😄

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

LGTM, this will land after the usual 48 hours 🕐

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 PR seems to be based on the #3963, but it was already addressed by documenting the behaviour of the socket option. There are lots of options, will we eventually have an example for every one of the dozen options? If yes, that seems excessive to me. If no, why is this particular option singled out?

@@ -821,6 +821,28 @@ The `callback` function, if specified, will be added as a listener for the

`tls.connect()` returns a [`tls.TLSSocket`][] object.

To upgrade an existing instance of `net.Socket` to a
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 documented above, does it really need an example? was the existing description of the socket: option somehow deficient?

const tls = require('tls');
const socket = new Socket();
const secureSock = tls.connect({ socket }, () => {
console.log('The TLS socket has been connected.');
Copy link
Contributor

Choose a reason for hiding this comment

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

at this point, neither the underlying Socket or tls.TLSSocket are connected, so the log message is misleading (see description of the socket option)

Copy link
Member

@tniessen tniessen Jul 5, 2017

Choose a reason for hiding this comment

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

This must have slipped through, earlier commits had the actual connect code in them, so the message was justified at that point.

@sam-github
Copy link
Contributor

sam-github commented Jul 5, 2017

Also, this PR doesn't address #3963, which is that the connect() doesn't actually connect the underlying TCP socket, causing the example code to exit immediately, and the example code here does indeed have the behaviour described in #3963, it exits immediately:

/tmp % node --version
v8.0.0
/tmp % node _.js
/tmp % cat _.js
const { Socket } = require('net');
const tls = require('tls');
const socket = new Socket();
const secureSock = tls.connect({ socket }, () => {
  console.log('The TLS socket has been connected.');
});

@VerteDinde
Copy link
Contributor Author

VerteDinde commented Jul 5, 2017

Hey @sam-github : Just want to make sure I understand your position - are you advocating for:

  1. Closing Issue (new TLSSocket(new net.Socket())).connect() fails silently. #3963 and this PR, because the issue is already adequately explained in the documentation, and getting more specific would require explaining dozens of other cases?
    or
  2. Altering this PR so that it more accurately addresses the issue raised in (new TLSSocket(new net.Socket())).connect() fails silently. #3963?

Just want to make sure I'm clear before I start making any changes. Happy to go either way, or take a third option if neither of the two above are adequately stating your thoughts. Thanks for your comments!

@sam-github
Copy link
Contributor

@VerteDinde IMO, 1. makes sense, if you arrived at this by looking at old issues for something to help out with. I am sorry that you wasted your time :-(.

If you got here because you found the current doc text unclear, that would be different, then it would be useful to clarify the behaviour that confused you.

@VerteDinde
Copy link
Contributor Author

@sam-github Oh no worries at all! I did arrive here by poking through old issues - I'm just a big fan of Node and wanted to give back and start contributing. 😄

I'm completely fine closing this PR and the issue and finding something else to help with. Learned a lot about TLS and sockets in the process, so it was a good use of time for me (though sorry/thanks to @tniessen for all his time and patience!). If there's something else I can help on, let me know, otherwise I'll poke through and find a simpler doc problem.

@sam-github
Copy link
Contributor

#881 (comment) <--- why not a binding for process.getuid()/geteuid()? Or if you aren't comfortable with C++, isTTY documented only in prose, not as a property (see the TTY docs), easy to fix.

@sam-github
Copy link
Contributor

random stuff I've seen and not had time to fix. warning some of these may not longer be true.

@vsemozhetbyt
Copy link
Contributor

Cross-refs) #14085 (comment) ...

@tniessen
Copy link
Member

tniessen commented Jul 9, 2017

@sam-github Please close this if you feel like it should not be landed for your presented reasons.

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

Successfully merging this pull request may close these issues.

8 participants