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

src: fix error message in async_hooks constructor #19000

Closed
wants to merge 2 commits into from

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

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

@@ -45,9 +45,9 @@ class AsyncHook {
if (before !== undefined && typeof before !== 'function')
throw new errors.TypeError('ERR_ASYNC_CALLBACK', 'before');
Copy link
Member

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

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


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

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

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

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

@danbev
Copy link
Contributor Author

danbev commented Feb 27, 2018

@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
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
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
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 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.

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
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

6 participants