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: use smart pointer in AsyncWrap::WeakCallback #19168

Closed

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

@@ -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

@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
PR-URL: #19168
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@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
PR-URL: #19168
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@targos targos mentioned this pull request Mar 18, 2018
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
PR-URL: #19168
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#19168
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@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
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
Development

Successfully merging this pull request may close these issues.

7 participants