async-hooks,net: ensure asyncId=null if no handle #13938

baudehlo · 2017-06-26T22:06:27Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
- Note this doesn't pass, but that's because master was failing before. The commit I added passes.
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

async-hooks, net

baudehlo · 2017-06-26T22:06:56Z

addaleax

Can you make a few edits? Also, it would be perfect if you could include Fixes: https://github.com/nodejs/node/issues/13548 in the commit message itself so that the issue will automatically be closed when this is merged.

Thank you for doing this!

@nodejs/async_hooks

addaleax · 2017-06-26T22:07:43Z

test/async-hooks/test-get-connections.js

+try {
+    server.getConnections(() => {
+        assert.ok(true, "No error thrown")
+    });


Can you indent using 2 spaces? I think our linter complains otherwise.

addaleax · 2017-06-26T22:07:53Z

test/async-hooks/test-get-connections.js

@@ -0,0 +1,14 @@
+'use strict';
+
+const assert = require('assert');


Can you const common = require('../common'); here?

addaleax · 2017-06-26T22:08:57Z

test/async-hooks/test-get-connections.js

+
+try {
+    server.getConnections(() => {
+        assert.ok(true, "No error thrown")


You can just call server.getConnections(common.mustCall()) or server.getConnections(common.mustCall(fn)), that’s a bit shorter (and actually makes sure that the callback gets executed) :)

addaleax · 2017-06-26T22:09:23Z

test/async-hooks/test-get-connections.js

+}
+catch (e) {
+    assert.ok(false, "getConnections threw an error");
+}


You don’t need the try/catch, if an error is thrown the test is going to fail anyway because the process crashes with an uncaught exception

OK, would you prefer a comment to that effect then? I was trying to write the test showing the context of what happened before the change.

refack

LGTM % previous comments (to get linter passing)

baudehlo · 2017-06-27T01:19:45Z

Will address the edits. I think adding the Fixes line in a comment works with github too fwiw.

refack · 2017-06-27T01:49:34Z

test/async-hooks/test-get-connections.js

+const net = require('net');
+const server = net.createServer();
+
+/* This test was based on an error raised by Haraka.


The comment is good 👍
Style nit: line comments (//) are prefered.
And replace See: with Ref:

refack · 2017-06-27T01:50:15Z

test/async-hooks/test-get-connections.js

+'use strict';
+
+const common = require('../common');
+const assert = require('assert');


unused assert will fail linter.

refack · 2017-06-27T01:52:00Z

CI: https://ci.nodejs.org/job/node-test-commit/10779/
Lint fail:

not ok 3 - /usr/home/iojs/build/workspace/node-test-linter/test/async-hooks/test-get-connections.js
  ---
  message: '''assert'' is assigned a value but never used.'
  severity: error
  data:
    line: 4
    column: 7
    ruleId: no-unused-vars
  messages:
    - message: Trailing spaces not allowed.
      severity: error
      data:
        line: 10
        column: 1
        ruleId: no-trailing-spaces
  ...

mscdex · 2017-06-27T06:03:46Z

I guess this is the kind of change needed for http.Agent as well?

AndreasMadsen · 2017-06-27T07:03:58Z

test/async-hooks/test-get-connections.js

@@ -0,0 +1,11 @@
+'use strict';


Hi, please rename the file to test-net-get-connections.js.

AndreasMadsen · 2017-07-03T08:28:01Z

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

@baudehlo will you rename the file and add Fixes: https://github.com/nodejs/node/issues/13548 to the commit message. If not, I can do it myself.

baudehlo · 2017-07-03T11:47:26Z

Just moved house and have no internet until Friday. So could you do it?

…

On Jul 3, 2017, at 4:28 AM, Andreas Madsen ***@***.***> wrote: CI: https://ci.nodejs.org/job/node-test-pull-request/8942/ @baudehlo will you rename the file. If not, I can do it myself. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

If the .listen() hasn't been called on the server, there is no handle object. In this case use null as the triggerAsyncId. Fixes: nodejs#13548

AndreasMadsen · 2017-07-03T12:01:13Z

Sure.

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

AndreasMadsen

LGTM

trevnorris

Thank you

AndreasMadsen · 2017-07-05T13:53:49Z

landed in aa8655a

tniessen · 2017-07-05T13:58:33Z

I think it should have been async_hooks in the commit title. not async-hooks. Just for the future.

AndreasMadsen · 2017-07-05T14:07:59Z

@tniessen sorry about that.

If the .listen() hasn't been called on the server, there is no handle object. In this case use null as the triggerAsyncId. Fixes: #13548 PR-URL: #13938 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Andreas Madsen <[email protected]>

nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Jun 26, 2017

addaleax reviewed Jun 26, 2017

View reviewed changes

addaleax added the async_hooks Issues and PRs related to the async hooks subsystem. label Jun 26, 2017

refack approved these changes Jun 26, 2017

View reviewed changes

jasnell approved these changes Jun 26, 2017

View reviewed changes

refack approved these changes Jun 27, 2017

View reviewed changes

refack reviewed Jun 27, 2017

View reviewed changes

AndreasMadsen reviewed Jun 27, 2017

View reviewed changes

Matt Sergeant and others added 4 commits July 3, 2017 13:56

async-hooks,net: ensure asyncId=null if no handle

b68cc5c

If the .listen() hasn't been called on the server, there is no handle object. In this case use null as the triggerAsyncId. Fixes: nodejs#13548

[squash] Address PR requests

bbdc7c1

[squash] More cleanups from PR comments

fc5a7fc

[squash] rename test

6ccf034

AndreasMadsen force-pushed the issue-13548 branch from a968e81 to 6ccf034 Compare July 3, 2017 12:00

AndreasMadsen added dont-land-on-v4.x labels Jul 3, 2017

AndreasMadsen approved these changes Jul 3, 2017

View reviewed changes

refack approved these changes Jul 3, 2017

View reviewed changes

trevnorris approved these changes Jul 3, 2017

View reviewed changes

AndreasMadsen closed this Jul 5, 2017

addaleax mentioned this pull request Jul 11, 2017

v8.2.0 proposal #13744

Merged

msimerson mentioned this pull request Jul 25, 2017

Haraka doesn't start. haraka/Haraka#2005

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async-hooks,net: ensure asyncId=null if no handle #13938

async-hooks,net: ensure asyncId=null if no handle #13938

baudehlo commented Jun 26, 2017 •

edited

Loading

baudehlo commented Jun 26, 2017

addaleax left a comment •

edited

Loading

addaleax Jun 26, 2017

addaleax Jun 26, 2017

addaleax Jun 26, 2017

addaleax Jun 26, 2017

baudehlo Jun 27, 2017

refack left a comment

baudehlo commented Jun 27, 2017

refack Jun 27, 2017

refack Jun 27, 2017

refack commented Jun 27, 2017

mscdex commented Jun 27, 2017

AndreasMadsen Jun 27, 2017

AndreasMadsen commented Jul 3, 2017 •

edited

Loading

baudehlo commented Jul 3, 2017 via email

AndreasMadsen commented Jul 3, 2017

AndreasMadsen left a comment

trevnorris left a comment

AndreasMadsen commented Jul 5, 2017

tniessen commented Jul 5, 2017

AndreasMadsen commented Jul 5, 2017

		@@ -0,0 +1,14 @@
		'use strict';

		const assert = require('assert');

async-hooks,net: ensure asyncId=null if no handle #13938

async-hooks,net: ensure asyncId=null if no handle #13938

Conversation

baudehlo commented Jun 26, 2017 • edited Loading

Checklist

Affected core subsystem(s)

baudehlo commented Jun 26, 2017

addaleax left a comment • 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

refack left a comment

Choose a reason for hiding this comment

baudehlo commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jun 27, 2017

mscdex commented Jun 27, 2017

Choose a reason for hiding this comment

AndreasMadsen commented Jul 3, 2017 • edited Loading

baudehlo commented Jul 3, 2017 via email

AndreasMadsen commented Jul 3, 2017

AndreasMadsen left a comment

Choose a reason for hiding this comment

trevnorris left a comment

Choose a reason for hiding this comment

AndreasMadsen commented Jul 5, 2017

tniessen commented Jul 5, 2017

AndreasMadsen commented Jul 5, 2017

baudehlo commented Jun 26, 2017 •

edited

Loading

addaleax left a comment •

edited

Loading

AndreasMadsen commented Jul 3, 2017 •

edited

Loading