Skip to content

src: fix error message in async_hooks constructor#19000

Closed
danbev wants to merge 2 commits intonodejs:masterfrom
danbev:async_hooks_error_msg
Closed

src: fix error message in async_hooks constructor#19000
danbev wants to merge 2 commits intonodejs:masterfrom
danbev:async_hooks_error_msg

Conversation

@danbev
Copy link
Contributor

@danbev danbev commented Feb 26, 2018

There are two minor issues in the AsyncHook constructor, if the object
passed in has an after and/or destroy property that are not functions
the errors thrown will still be:

TypeError [ERR_ASYNC_CALLBACK]: before must be a function

This commit updates the code and adds a unit test.

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)

src

@danbev
src: fix error message in async_hooks constructor
df5ee8f 
There are two minor issues in the AsyncHook constructor, if the object
passed in has an after and/or destroy property that are not functions
the errors thrown will still be:
TypeError [ERR_ASYNC_CALLBACK]: before must be a function

This commit updates the code and adds a unit test.
@nodejs-github-bot nodejs-github-bot added the async_hooks Issues and PRs related to the async hooks subsystem. label Feb 26, 2018
@danbev
Copy link
Contributor Author

danbev commented Feb 26, 2018

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

TimothyGu
TimothyGu reviewed Feb 26, 2018
View reviewed changes
lib/async_hooks.js Outdated
@@ -45,9 +45,9 @@ class AsyncHook {
if (before !== undefined && typeof before !== 'function')
throw new errors.TypeError('ERR_ASYNC_CALLBACK', 'before');
Copy link
Member

@TimothyGu TimothyGu Feb 26, 2018

Choose a reason for hiding this comment

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

How about passing 'hook.before' as the option name? Usually a bare identifier means a function parameter.

Copy link
Contributor Author

@danbev danbev Feb 26, 2018

Choose a reason for hiding this comment

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

Sounds good, I'll take a look. Thanks

test/parallel/test-async-hooks-constructor.js Outdated

common.expectsError(() => {
async_hooks.createHook({ promiseResolve: non_function });
}, typeErrorForFunction('promiseResolve'));
Copy link
Member

@TimothyGu TimothyGu Feb 26, 2018

Choose a reason for hiding this comment

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

typeErrorForFunction could contain the common.expectsError logic as well.

Copy link
Contributor Author

@danbev danbev Feb 26, 2018

Choose a reason for hiding this comment

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

Sounds good as well, I update this. Thanks

jasnell
jasnell approved these changes Feb 26, 2018
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with the suggested changes.

@danbev
Copy link
Contributor Author

danbev commented Feb 27, 2018

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

@danbev danbev force-pushed the async_hooks_error_msg branch from ef785fa to 8a43ed1 Compare February 27, 2018 08:34
@danbev
Copy link
Contributor Author

danbev commented Feb 27, 2018

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

@mmarchini mmarchini added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 27, 2018
@danbev
Copy link
Contributor Author

danbev commented Feb 28, 2018

node-test-commit failure looks unrelated

console output:

not ok 1043 parallel/test-https-host-headers
  ---
  duration_ms: 1.101
  severity: fail
  stack: |-
    test https server listening on port 45571
    Got request: localhost:45571 /0
    /home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64/test/parallel/test-https-host-headers.js:32
      throw er;
      ^
    
    Error: 281472848347136:error:1408F119:SSL routines:SSL3_GET_RECORD:decryption failed or bad record mac:../deps/openssl/openssl/ssl/s3_pkt.c:535:
    
  ...

@danbev
Copy link
Contributor Author

danbev commented Feb 28, 2018

Landed in f2defca.

@danbev danbev closed this Feb 28, 2018
@danbev danbev deleted the async_hooks_error_msg branch February 28, 2018 06:41
danbev added a commit that referenced this pull request Feb 28, 2018
@danbev
src: fix error message in async_hooks constructor
f2defca 
There are two minor issues in the AsyncHook constructor, if the object
passed in has an after and/or destroy property that are not functions
the errors thrown will still be:
TypeError [ERR_ASYNC_CALLBACK]: before must be a function

This commit updates the code and adds a unit test.

PR-URL: #19000
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
addaleax pushed a commit to addaleax/node that referenced this pull request Mar 5, 2018
@danbev @addaleax
src: fix error message in async_hooks constructor
4d5cd5c 
There are two minor issues in the AsyncHook constructor, if the object
passed in has an after and/or destroy property that are not functions
the errors thrown will still be:
TypeError [ERR_ASYNC_CALLBACK]: before must be a function

This commit updates the code and adds a unit test.

PR-URL: nodejs#19000
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 6, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@danbev @MayaLekova
src: fix error message in async_hooks constructor
0a60dd5 
There are two minor issues in the AsyncHook constructor, if the object
passed in has an after and/or destroy property that are not functions
the errors thrown will still be:
TypeError [ERR_ASYNC_CALLBACK]: before must be a function

This commit updates the code and adds a unit test.

PR-URL: nodejs#19000
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
jasnell pushed a commit to jasnell/node that referenced this pull request Aug 17, 2018
@danbev @jasnell
src: fix error message in async_hooks constructor
6456de8 
There are two minor issues in the AsyncHook constructor, if the object
passed in has an after and/or destroy property that are not functions
the errors thrown will still be:
TypeError [ERR_ASYNC_CALLBACK]: before must be a function

This commit updates the code and adds a unit test.

PR-URL: nodejs#19000
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 6, 2018
@danbev @MylesBorins
src: fix error message in async_hooks constructor
02ea033 
There are two minor issues in the AsyncHook constructor, if the object
passed in has an after and/or destroy property that are not functions
the errors thrown will still be:
TypeError [ERR_ASYNC_CALLBACK]: before must be a function

This commit updates the code and adds a unit test.

Backport-PR-URL: #22380
PR-URL: #19000
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 6, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

+2 more reviewers

@TimothyGu TimothyGu TimothyGu approved these changes

@mmarchini mmarchini mmarchini approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@danbev @jasnell @TimothyGu @cjihrig @mmarchini @nodejs-github-bot