doc: net.Socket in TLSSocket #8996

minervapanda · 2016-10-09T18:54:20Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

tls

Description of change

If we don't wrap the net.Socket in a TLSSocket, then the connect works as expected.But if we're using TLS from the start, then we just use tls.connect().

mscdex · 2016-10-09T21:31:21Z

It looks like you have an extra file committed that shouldn't be there.

mscdex · 2016-10-09T21:31:40Z

doc/api/tls.md

@@ -885,7 +885,6 @@ socket.on('end', () => {
  server.close();
 });
 ```
-


Unnecessary whitespace change.

minervapanda · 2016-10-11T09:35:46Z

@mscdex I worked on it. Is it okay now ? Please let me know ?

mscdex · 2016-10-11T10:54:27Z

@minervapanda Almost there. It looks like there's still a couple of unnecessary blank line removals.

minervapanda · 2016-10-11T12:04:02Z

@mscdex Sorry I could not know it looked fine to me.

mscdex · 2016-10-11T18:11:19Z

Sorry, I meant in the actual markdown itself, not the rendered output.

gibfahn · 2016-10-12T13:18:01Z

@minervapanda You can see them here: https://github.com/nodejs/node/pull/8996/files

sam-github · 2016-10-18T20:39:55Z

doc/api/tls.md

@@ -911,6 +909,21 @@ socket.on('end', () => {
 });
 ```

+Note from the above example that if you don't wrap the `net.Socket` in a


I don't understand what the paragraph is trying to say, or the example below is trying to show. What are you trying to describe?

You mention the "above example", but it doesn't create net sockets and wrap them, so how is this a note about the above example?

Are you trying to describe the somewhat weird behaviour that you can call tls.connect() before calling sock.connect(), with wrapped sockets, and it all works out?

Or are you just trying to give an example of using the socket: option to tls.connect(), since that option isn't used in the example?

Or something else?

sam-github · 2016-10-18T20:40:46Z

doc/api/tls.md

+var Socket = require('net').Socket;
+var tls = require('tls');
+var sock = new Socket();
+var secureSock = tls.connect({ socket: s }, function() {


Inconsistent object literal spacing, should be {socket (no space), as it is below. Same for }.

Fixes: nodejs#3963

sam-github

You changed the file modes of hundreds of files and committed them, so the PR is damaged, you'll have to back those bits out.

Sorry, -1 on this PR. I found it very difficult to figure out what it is you are trying to describe.

I've read your code over a couple times, and I have a guess: you thought that calling tls.connect() would start not just the TLS, but also call connect on the socket you provided. Can you confirm this?

If so, this isn't the way to document that. Please modify the docs for the socket option to tls.connect to say that it is the callers responsibility to manage the socket, including connecting it. Or I can write the docs for that, if you like?

Replace nodejs#8996

Document all TLSSocket options: - All the secure context options are valid options to a secureContext - isServer modifies the default value of requestCert Describe all tls.connect() variants: - tls.connect(path) was undocumented - tls.connect(port) was underdocumented, and its relationship to tls.connect(options) was obscure Socket passed to tls.connect is user managed: - Replace nodejs#8996 Add documentation to: - describe and add tests for the pfx and key variants, and describe how and when passphrase is used. - describe tls cert and ca options - describe buffer forms of tls crl option - describe tls cipher option and defaults - fix link to Crypto Constants - describe that honorCipherOrder sets SSL_OP_CIPHER_SERVER_PREFERENCE. - describe tls ecdhCurve/dhparam options - describe tls secureProtocol option - describe tls secureOptions - describe tls sessionIdContext De-deduplicate secure context docs: The secure context options were documented 4 times, making it difficult to understand where the options come from, where they are supported, and under what conditions they are used. The multiple copies were inconsistent and contradictory in their descriptions of the options, and also inconsistent in whether the options would be documented at all. Cut through this gordian knot by linking all APIs that use the secureContext options to the single source of truth about the options. PR-URL: nodejs#9800 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Document all TLSSocket options: - All the secure context options are valid options to a secureContext - isServer modifies the default value of requestCert Describe all tls.connect() variants: - tls.connect(path) was undocumented - tls.connect(port) was underdocumented, and its relationship to tls.connect(options) was obscure Socket passed to tls.connect is user managed: - Replace #8996 Add documentation to: - describe and add tests for the pfx and key variants, and describe how and when passphrase is used. - describe tls cert and ca options - describe buffer forms of tls crl option - describe tls cipher option and defaults - fix link to Crypto Constants - describe that honorCipherOrder sets SSL_OP_CIPHER_SERVER_PREFERENCE. - describe tls ecdhCurve/dhparam options - describe tls secureProtocol option - describe tls secureOptions - describe tls sessionIdContext De-deduplicate secure context docs: The secure context options were documented 4 times, making it difficult to understand where the options come from, where they are supported, and under what conditions they are used. The multiple copies were inconsistent and contradictory in their descriptions of the options, and also inconsistent in whether the options would be documented at all. Cut through this gordian knot by linking all APIs that use the secureContext options to the single source of truth about the options. PR-URL: #9800 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

fhinkel · 2017-05-26T19:12:09Z

There hasn't been any activity here. I'm closing this. Feel free to reopen (or ping a collaborator) if I closed this in error.

minervapanda force-pushed the pr3 branch 2 times, most recently from 50bfcd6 to 3e442c3 Compare October 9, 2016 19:00

mscdex reviewed Oct 9, 2016

View reviewed changes

doc/api/tls.md

@@ -885,7 +885,6 @@ socket.on('end', () => {

server.close();

});

```

Copy link

Contributor

mscdex Oct 9, 2016

Choose a reason for hiding this comment

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

Unnecessary whitespace change.

mscdex added tls Issues and PRs related to the tls subsystem. doc Issues and PRs related to the documentations. labels Oct 9, 2016

minervapanda force-pushed the pr3 branch 3 times, most recently from 32fbc28 to c94440b Compare October 11, 2016 09:33

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

sam-github reviewed Oct 18, 2016

View reviewed changes

doc: net.Socket in TLSSocket

8cf20ca

Fixes: nodejs#3963

minervapanda force-pushed the pr3 branch from e40847a to 8cf20ca Compare November 7, 2016 19:11

sam-github suggested changes Nov 18, 2016

View reviewed changes

sam-github added a commit to sam-github/node that referenced this pull request Dec 9, 2016

doc: socket passed to tls.connect is user managed

c94b954

Replace nodejs#8996

jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

fhinkel closed this May 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: net.Socket in TLSSocket #8996

doc: net.Socket in TLSSocket #8996

minervapanda commented Oct 9, 2016

mscdex commented Oct 9, 2016 •

edited

Loading

mscdex Oct 9, 2016

minervapanda commented Oct 11, 2016

mscdex commented Oct 11, 2016

minervapanda commented Oct 11, 2016

mscdex commented Oct 11, 2016

gibfahn commented Oct 12, 2016

sam-github Oct 18, 2016

sam-github Oct 18, 2016

sam-github left a comment

fhinkel commented May 26, 2017

doc: net.Socket in TLSSocket #8996

doc: net.Socket in TLSSocket #8996

Conversation

minervapanda commented Oct 9, 2016

Checklist

Affected core subsystem(s)

Description of change

mscdex commented Oct 9, 2016 • edited Loading

mscdex Oct 9, 2016

Choose a reason for hiding this comment

minervapanda commented Oct 11, 2016

mscdex commented Oct 11, 2016

minervapanda commented Oct 11, 2016

mscdex commented Oct 11, 2016

gibfahn commented Oct 12, 2016

sam-github Oct 18, 2016

Choose a reason for hiding this comment

sam-github Oct 18, 2016

Choose a reason for hiding this comment

sam-github left a comment

Choose a reason for hiding this comment

fhinkel commented May 26, 2017

mscdex commented Oct 9, 2016 •

edited

Loading