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

async_hooks: add HandleScopes to C++ embedder/addon API #24285

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

Add HandleScopes to the public C++ API for embedders/addons,
since these methods create V8 handles that should not leak into
the outer scopes.

In particular, for some of the methods it was not clear from
the function signatures that these functions previously
needed to be invoked with a HandleScope.

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

Add `HandleScope`s to the public C++ API for embedders/addons,
since these methods create V8 handles that should not leak into
the outer scopes.

In particular, for some of the methods it was not clear from
the function signatures that these functions previously
needed to be invoked with a `HandleScope`.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 10, 2018
@addaleax addaleax added the async_hooks Issues and PRs related to the async hooks subsystem. label Nov 10, 2018
@addaleax
Copy link
Member Author

@addaleax
Copy link
Member Author

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 10, 2018
@joyeecheung
Copy link
Member

Failures in job https://ci.nodejs.org/job/node-test-pull-request/18507/

ubuntu1604_sharedlibs_debug_x64

See failures on test-joyent-ubuntu1604_sharedlibs_container-x64-4:
not ok 2051 parallel/test-tls-net-socket-keepalive
  ---
  duration_ms: 480.68
  severity: fail
  exitcode: -15
  stack: |-
    timeout
  ...

ubuntu1604_sharedlibs_openssl110_x64

See failures on test-digitalocean-ubuntu1604_sharedlibs_container-x64-6:
not ok 2160 parallel/test-tls-net-socket-keepalive
  ---
  duration_ms: 120.54
  severity: fail
  exitcode: -15
  stack: |-
    timeout
  ...

ubuntu1604_sharedlibs_shared_x64

See failures on test-digitalocean-ubuntu1604_sharedlibs_container-x64-4:
not ok 2310 parallel/test-tls-net-socket-keepalive
  ---
  duration_ms: 120.33
  severity: fail
  exitcode: -15
  stack: |-
    timeout
  ...

ubuntu1604_sharedlibs_withoutintl_x64

See failures on test-digitalocean-ubuntu1604_sharedlibs_container-x64-10:
not ok 2311 parallel/test-tls-net-socket-keepalive
  ---
  duration_ms: 120.75
  severity: fail
  exitcode: -15
  stack: |-
    timeout
  ...

ubuntu1604_sharedlibs_zlib_x64

See failures on test-digitalocean-ubuntu1604_sharedlibs_container-x64-2:
not ok 2324 parallel/test-tls-net-socket-keepalive
  ---
  duration_ms: 120.93
  severity: fail
  exitcode: -15
  stack: |-
    timeout
  ...

Resume: https://ci.nodejs.org/job/node-test-pull-request/18531/

@addaleax
Copy link
Member Author

@joyeecheung I think that’s my fault (#24288) 😄

Resume CI again: https://ci.nodejs.org/job/node-test-pull-request/18531/

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax
Copy link
Member Author

Landed in fcd7a72

@addaleax addaleax closed this Nov 13, 2018
@addaleax addaleax deleted the async-handle-scopes branch November 13, 2018 15:02
addaleax added a commit that referenced this pull request Nov 13, 2018
Add `HandleScope`s to the public C++ API for embedders/addons,
since these methods create V8 handles that should not leak into
the outer scopes.

In particular, for some of the methods it was not clear from
the function signatures that these functions previously
needed to be invoked with a `HandleScope`.

PR-URL: #24285
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
Add `HandleScope`s to the public C++ API for embedders/addons,
since these methods create V8 handles that should not leak into
the outer scopes.

In particular, for some of the methods it was not clear from
the function signatures that these functions previously
needed to be invoked with a `HandleScope`.

PR-URL: #24285
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
Add `HandleScope`s to the public C++ API for embedders/addons,
since these methods create V8 handles that should not leak into
the outer scopes.

In particular, for some of the methods it was not clear from
the function signatures that these functions previously
needed to be invoked with a `HandleScope`.

PR-URL: nodejs#24285
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
codebytere pushed a commit that referenced this pull request Jan 12, 2019
Add `HandleScope`s to the public C++ API for embedders/addons,
since these methods create V8 handles that should not leak into
the outer scopes.

In particular, for some of the methods it was not clear from
the function signatures that these functions previously
needed to be invoked with a `HandleScope`.

PR-URL: #24285
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@codebytere codebytere mentioned this pull request Jan 15, 2019
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. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants