async_wrap: add `asyncReset` to `TLSWrap` #13092

refack · 2017-05-18T04:44:28Z

When using an Agent for HTTPS, TLSSockets are reused and need to
have the ability to asyncReset from JS.

Fixes: #13045
(Not a complete solution. Does not solve custom Agent use case)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

async_wrap

refack · 2017-05-18T04:46:40Z

CI: https://ci.nodejs.org/job/node-test-commit/9967/
(just cause my computer takes so long to build...)

refack · 2017-05-18T10:18:03Z

New CI: https://ci.nodejs.org/job/node-test-commit/9974/

refack · 2017-05-18T10:19:17Z

/cc @Fishrock123 @AndreasMadsen @mscdex @addaleax @trevnorris PTAL

AndreasMadsen · 2017-05-18T10:38:20Z

test/parallel/test-async-wrap-GH13045.js

+
+  const req = https.request(clientOptions, common.mustCall((res) => {
+    assert.strictEqual(res.statusCode, 200);
+    res.on('error', common.mustNotCall());


Is there a reason to add this listener? If there is an error in the future I think it would be better to see that error than an AssertionError('must not throw') message.

🤔 and put assert.ifError?
Ok.

It will just throw by default, which will cause the test to fail.

But then you don't get which line... And since there are two almost identical calls it's a PITA (just been there while writing this test)

I see. I think the ifError approach is fine then.

refack · 2017-05-18T10:50:09Z

Fresh CI: https://ci.nodejs.org/job/node-test-commit/9975/

AndreasMadsen

LGTM

cjihrig

A couple of nits with the test, but mostly LGTM

cjihrig · 2017-05-18T14:12:18Z

test/parallel/test-async-wrap-GH13045.js

+'use strict';
+const common = require('../common');
+
+// checks for the issue in https://github.com/nodejs/node/issues/13045


checks for the issue in -> Refs:

cjihrig · 2017-05-18T14:13:59Z

test/parallel/test-async-wrap-GH13045.js

+
+  const req = https.request(clientOptions, common.mustCall((res) => {
+    assert.strictEqual(res.statusCode, 200);
+    res.on('error', assert.ifError);


Can you replace the uses of assert.ifError() with common.mustNotCall(). You can provide a custom message with the latter if you'd like.

See #13092 (comment)
@AndreasMadsen suggested that we might want to see the actual error.
(Maybe we need mustNotErr like assert.doesNotThrow)

OK, you might want to use assert.fail() instead. ifError() implies that there might not be an error, but in this case, we know that there is one.

Added a factory method mustNotErr that calls assert.fail so the fail point will appear in the stack

cjihrig · 2017-05-18T14:14:40Z

test/parallel/test-async-wrap-GH13045.js

+      try {
+        req2 = https.request(clientOptions);
+      } catch (e) {
+        assert.ifError(e);


Isn't this redundant?

In a sense yes, I just wanted to be explicit about the pain point.
I could replace this with a comment.

Couldn't you drop the try...catch completely, and if it happens to throw, it will fail the test.

Ack. Replaced with a comment

cjihrig · 2017-05-18T14:57:48Z

test/parallel/test-async-wrap-GH13045.js

+'use strict';
+const common = require('../common');
+
+// Checks for the issue in Refs.


Sorry if my previous comment was unclear. I just meant to write this as:

Refs: #13045

cjihrig · 2017-05-18T14:58:54Z

test/parallel/test-async-wrap-GH13045.js

+const fs = require('fs');
+
+// arrow factory so fail point will appear in the stack
+function mustNotErr() {


I don't understand why you need this. You can just pass assert.fail (without the parens) as the handler, everywhere you use mustNotErr().

Tried it, then there is no frame for the line with the failing assert (Actually the factory is not a solution I need explicit (err) => assert.fail(err))

Generated error by setting port: port + 1
with (err) => assert.fail(err)

assert.js:92 throw new AssertionError({ ^ AssertionError [ERR_ASSERTION]: Error: write EPROTO 101057795:error:140770FC:SSL routines:SSL23_GET_SERVER_HELLO:unknown protocol:openssl\ssl\s23_clnt.c:794: at ClientRequest.req.on (D:\code\node-cur\test\parallel\test-async-wrap-GH13045.js:63:35) at emitOne (events.js:115:13) at ClientRequest.emit (events.js:210:7) at TLSSocket.socketErrorListener (_http_client.js:397:9) at emitOne (events.js:115:13) at TLSSocket.emit (events.js:210:7) at onwriteError (_stream_writable.js:359:10) at onwrite (_stream_writable.js:377:5) at fireErrorCallbacks (net.js:522:13) at TLSSocket.Socket._destroy (net.js:563:3)

with just assert.fail

assert.js:92 throw new AssertionError({ ^ AssertionError [ERR_ASSERTION]: Error: write EPROTO 101057795:error:140770FC:SSL routines:SSL23_GET_SERVER_HELLO:unknown protocol:openssl\ssl\s23_clnt.c:794: at emitOne (events.js:115:13) at ClientRequest.emit (events.js:210:7) at TLSSocket.socketErrorListener (_http_client.js:397:9) at emitOne (events.js:115:13) at TLSSocket.emit (events.js:210:7) at onwriteError (_stream_writable.js:359:10) at onwrite (_stream_writable.js:377:5) at fireErrorCallbacks (net.js:522:13) at TLSSocket.Socket._destroy (net.js:563:3) at WriteWrap.afterWrite [as oncomplete] (net.js:857:10)

Fishrock123 · 2017-05-18T15:32:29Z

Sounds correct to me.

mcollina · 2017-05-18T16:02:15Z

Can we get a CITGM run? that has been the main blocker for me.

addaleax · 2017-05-18T16:09:34Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/795/

mscdex · 2017-05-18T18:17:02Z

test/parallel/test-async-wrap-GH13045.js

+    });
+  }));
+
+  req.on('error', (err) => assert.fail(err));


Isn't this more or less equivalent to just not adding an 'error' listener? Similarly with the 'error' handlers above.

See #13092 (comment) and #13092 (comment)

In that case, I'm -0 on it.

mscdex · 2017-05-18T18:18:35Z

test/parallel/test-async-wrap-GH13045.js

+      // This is the pain point. Internally the Agent will call
+      // `socket._handle.asyncReset()` and if the _handle does not implement
+      // `asyncReset` this will throw TypeError
+      const req2 = https.request(clientOptions);


Add the response handler as a second argument here for consistency?

Ditto about switching to https.get().

mscdex · 2017-05-18T18:19:07Z

test/parallel/test-async-wrap-GH13045.js

+  ca: fs.readFileSync(`${common.fixturesDir}/keys/ca1-cert.pem`)
+};
+
+const server = https.createServer(serverOptions, (req, res) => {


common.mustCall((req, res) => { ... }, 2) ?

mscdex · 2017-05-18T18:20:04Z

test/parallel/test-async-wrap-GH13045.js

+    agent: new https.Agent({
+      keepAlive: true,
+      maxSockets: 50,
+      rejectUnauthorized: false,


Are these last two options necessary?

Just last one: AssertionError [ERR_ASSERTION]: Error: self signed certificate in certificate chain

mscdex · 2017-05-18T18:21:00Z

test/parallel/test-async-wrap-GH13045.js

+    method: 'GET'
+  };
+
+  const req = https.request(clientOptions, common.mustCall((res) => {


I think method: 'GET' could be dropped and .request() changed to .get() to avoid the need to req.end().

mscdex · 2017-05-18T18:21:27Z

test/parallel/test-async-wrap-GH13045.js

+    }),
+    hostname: common.localhostIPv4,
+    port: port,
+    path: '/',


This isn't needed, it's even the default.

mcollina

LGTM

refack · 2017-05-19T12:22:58Z

CI: https://ci.nodejs.org/job/node-test-commit/10000/

refack · 2017-05-19T16:18:39Z

Pre land CI:https://ci.nodejs.org/job/node-test-commit/10006/

Ping @trevnorris any comments? I would like to land this in a few hours.

refack · 2017-05-20T03:29:15Z

Landed in 6bfdeed

refack · 2017-05-20T03:30:30Z

Post land CI: https://ci.nodejs.org/job/node-test-commit/10031/

AndreasMadsen · 2017-05-20T06:42:22Z

@refack Thanks for taking care of this.

mcollina · 2017-05-20T06:47:09Z

Thank you @refack. 🎉

See: - npm/npm#16636 - nodejs/node#13092

MylesBorins · 2017-07-17T14:53:01Z

This should likely be included in a larger async_wrap backport if it were to happen

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem. labels May 18, 2017

refack added the async_wrap label May 18, 2017

refack self-assigned this May 18, 2017

mscdex added the wip Issues and PRs that are still a work in progress. label May 18, 2017

refack force-pushed the async-wrap-13045 branch from 077f403 to dcdc104 Compare May 18, 2017 10:13

refack changed the title ~~[WIP] test-balloon: investigate #13045~~ async_wrap: add asyncReset to TLSWrap May 18, 2017

refack removed the wip Issues and PRs that are still a work in progress. label May 18, 2017

refack requested a review from trevnorris May 18, 2017 10:18

refack mentioned this pull request May 18, 2017

http: improve outgoing string write performance #13013

Merged

2 tasks

refack requested a review from AndreasMadsen May 18, 2017 10:29

AndreasMadsen reviewed May 18, 2017

View reviewed changes

AndreasMadsen approved these changes May 18, 2017

View reviewed changes

cjihrig requested changes May 18, 2017

View reviewed changes

cjihrig approved these changes May 18, 2017

View reviewed changes

refack mentioned this pull request May 18, 2017

stream: add destroy and _destroy methods. #12925

Closed

4 tasks

mscdex reviewed May 18, 2017

View reviewed changes

mcollina approved these changes May 19, 2017

View reviewed changes

This was referenced May 19, 2017

doc: list macOS as supporting filename argument #13111

Closed

question: best practice for handling unexpected err in tests #13115

Closed

refack force-pushed the async-wrap-13045 branch from 47bd450 to ce6215e Compare May 19, 2017 16:16

refack force-pushed the async-wrap-13045 branch from ce6215e to 834e4cd Compare May 19, 2017 16:22

refack closed this May 20, 2017

refack force-pushed the async-wrap-13045 branch from 834e4cd to 6bfdeed Compare May 20, 2017 03:28

refack merged commit 6bfdeed into nodejs:master May 20, 2017

refack deleted the async-wrap-13045 branch May 20, 2017 03:33

sebdeckers mentioned this pull request May 22, 2017

http2: additional refinement of stream/session nodejs/http2#138

Closed

3 tasks

digitalinfinity mentioned this pull request May 22, 2017

npm does not work in latest nightly nodejs/node-chakracore#251

Closed

sebdeckers added a commit to sebdeckers/http2 that referenced this pull request May 25, 2017

Duplicating patch to fix NPM support.

4149e48

See: - npm/npm#16636 - nodejs/node#13092

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

refack removed their assignment Jun 12, 2017

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

refack mentioned this pull request Jun 20, 2017

test: investigate flaky test-http-agent-keepalive #13810

Closed

mscdex mentioned this pull request Jun 21, 2017

test-npm failing on master after introduction of initial async hooks implementation #13045

Closed

refack mentioned this pull request Jul 2, 2017

Cannot read property 'asyncReset' of null #13539

Closed

MylesBorins added the lts-watch-v6.x label Jul 17, 2017

MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 14, 2017

MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018

async_wrap: add asyncReset to TLSWrap #13092

async_wrap: add asyncReset to TLSWrap #13092

Conversation

refack commented May 18, 2017 • edited Loading

Checklist

Affected core subsystem(s)

refack commented May 18, 2017

refack commented May 18, 2017

refack commented May 18, 2017

AndreasMadsen May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented May 18, 2017

AndreasMadsen left a comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented May 18, 2017 • edited Loading

mcollina commented May 18, 2017

addaleax commented May 18, 2017

mscdex May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

refack commented May 19, 2017

refack commented May 19, 2017 • edited Loading

refack commented May 20, 2017

refack commented May 20, 2017

AndreasMadsen commented May 20, 2017

mcollina commented May 20, 2017

MylesBorins commented Jul 17, 2017

async_wrap: add `asyncReset` to `TLSWrap` #13092

async_wrap: add `asyncReset` to `TLSWrap` #13092

refack commented May 18, 2017 •

edited

Loading

AndreasMadsen May 18, 2017 •

edited

Loading

Fishrock123 commented May 18, 2017 •

edited

Loading

mscdex May 18, 2017 •

edited

Loading

mscdex May 18, 2017 •

edited

Loading

mscdex May 18, 2017 •

edited

Loading

refack commented May 19, 2017 •

edited

Loading