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.

Sign up for GitHub

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

Jump to bottom

src: use smart pointer in AsyncWrap::WeakCallback #19168

Closed
wants to merge 2 commits into from
Closed

src: use smart pointer in AsyncWrap::WeakCallback #19168

wants to merge 2 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Mar 6, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 6, 2018
@danbev
Copy link
Contributor Author

danbev commented Mar 6, 2018

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

addaleax
addaleax approved these changes Mar 6, 2018
View reviewed changes
src/async_wrap.cc
@@ -415,14 +415,13 @@ void AsyncWrap::WeakCallback(const v8::WeakCallbackInfo<DestroyParam>& info) {
HandleScope scope(info.GetIsolate());

Environment* env = Environment::GetCurrent(info.GetIsolate());
DestroyParam* p = info.GetParameter();
std::unique_ptr<DestroyParam> p{info.GetParameter()};
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Can you add a comment here that says that assigning the pointer will destroy the object at the end of the function?

I like smart pointers, but I think this is one of the cases where the ownership flow becomes a bit less obvious. I find it hard to put a finger on what exactly it is, but I think it’s mostly that we have a new call that creates the object and works with it as a raw pointer on the one end, and use a smart pointer on the other side (i.e. there’s no matching delete call)…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problems, I'll add a comment. Thanks

@BridgeAR
Copy link
Member

BridgeAR commented Mar 6, 2018

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

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. labels Mar 6, 2018
@danbev
Copy link
Contributor Author

danbev commented Mar 8, 2018

node-test-commit failure looks unrelated

console output:

not ok 1943 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-binary
  ---
  duration_ms: 26.585
  severity: crashed
  stack: |-
    oh no!
    exit code: CRASHED (Signal: 9)
  ...
not ok 1944 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-hex
  ---
  duration_ms: 29.225
  severity: crashed
  stack: |-
    oh no!
    exit code: CRASHED (Signal: 9)
  ...
node-test-commit-aix failure looks unrelated

console output:

not ok 1244 parallel/test-performance
  ---
  duration_ms: 2.838
  severity: fail
  stack: |-
    assert.js:249
        throw err;
        ^
    
    AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
    
      assert(Math.abs(delta) < 1000)
    
        at checkNodeTiming (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/test/parallel/test-performance.js:118:7)
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/test/parallel/test-performance.js:125:1)
        at Module._compile (module.js:670:30)
        at Object.Module._extensions..js (module.js:681:10)
        at Module.load (module.js:581:32)
        at tryModuleLoad (module.js:521:12)
        at Function.Module._load (module.js:513:3)
        at Function.Module.runMain (module.js:711:10)
        at startup (bootstrap_node.js:223:16)
        at bootstrapNodeJSCore (bootstrap_node.js:562:3)

@danbev
Copy link
Contributor Author

danbev commented Mar 8, 2018

Landed in 1e3dd8b.

@danbev danbev closed this Mar 8, 2018
danbev added a commit that referenced this pull request Mar 8, 2018
@danbev
src: use smart pointer in AsyncWrap::WeakCallback
1e3dd8b 
PR-URL: #19168
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@danbev danbev deleted the async_wrap_smart_pointer_weakcallback branch March 8, 2018 05:31
targos pushed a commit that referenced this pull request Mar 17, 2018
@danbev @targos
src: use smart pointer in AsyncWrap::WeakCallback
5dde091 
PR-URL: #19168
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@targos targos mentioned this pull request Mar 18, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@danbev @MylesBorins
src: use smart pointer in AsyncWrap::WeakCallback
64f6462 
PR-URL: #19168
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@danbev @MayaLekova
src: use smart pointer in AsyncWrap::WeakCallback
367947b 
PR-URL: nodejs#19168
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@jasnell
Copy link
Member

jasnell commented Aug 17, 2018

Should this be backported to 8.x? If so, a separate backport PR is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
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++. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@danbev @BridgeAR @jasnell @addaleax @cjihrig @joyeecheung @nodejs-github-bot