src: introduce custom smart pointers for `BaseObject`s by addaleax · Pull Request #30374 · nodejs/node · GitHub

Note: Some of the commits here are from https://github.com/nodejs/quic/, where this concept has proven useful so far.

src: introduce custom smart pointers for BaseObjects

Referring to BaseObject instances using standard C++ smart pointers
can interfere with BaseObject’s own cleanup mechanisms
(explicit delete, delete-on-GC and delete-on-cleanup).

Introducing custom smart pointers allows referring to BaseObjects
safely while keeping those mechanisms intact.

Refs: nodejs/quic#141
Refs: nodejs/quic#149
Reviewed-By: James M Snell jasnell@gmail.com

http2: use custom BaseObject smart pointers

Refs: nodejs/quic#141
Reviewed-By: James M Snell jasnell@gmail.com

src: use BaseObjectPtr for keeping channel alive in dns bindings

src: remove keep alive option from SetImmediate()

This is no longer necessary now that the copyable BaseObjectPtr
is available (as opposed to the only-movable v8::Global).

src: remove HandleWrap instances from list once closed

This allows keeping BaseObjectPtrs to HandleWrap instances.
Previously, the pointer kept the HandleWrap object alive, leaving
the Environment cleanup code that waits for the handle list to drain
in a busy loop, because only the HandleWrap destructor removed
the item from the list.

Refs: nodejs/quic#165
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Daniel Bevenius daniel.bevenius@gmail.com

Checklist

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

Referring to `BaseObject` instances using standard C++ smart pointers can interfere with BaseObject’s own cleanup mechanisms (explicit delete, delete-on-GC and delete-on-cleanup). Introducing custom smart pointers allows referring to `BaseObject`s safely while keeping those mechanisms intact. Refs: nodejs/quic#141 Refs: nodejs/quic#149 Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: nodejs/quic#141 Reviewed-By: James M Snell <jasnell@gmail.com>

This is no longer necessary now that the copyable `BaseObjectPtr` is available (as opposed to the only-movable `v8::Global`).

This allows keeping `BaseObjectPtr`s to `HandleWrap` instances. Previously, the pointer kept the `HandleWrap` object alive, leaving the Environment cleanup code that waits for the handle list to drain in a busy loop, because only the `HandleWrap` destructor removed the item from the list. Refs: nodejs/quic#165 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

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

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

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

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

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

Referring to `BaseObject` instances using standard C++ smart pointers can interfere with BaseObject’s own cleanup mechanisms (explicit delete, delete-on-GC and delete-on-cleanup). Introducing custom smart pointers allows referring to `BaseObject`s safely while keeping those mechanisms intact. Refs: nodejs/quic#141 Refs: nodejs/quic#149 Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: #30374 Refs: nodejs/quic#165 Reviewed-By: David Carlier <devnexen@gmail.com>

Refs: nodejs/quic#141 Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: #30374 Refs: nodejs/quic#149 Refs: nodejs/quic#165 Reviewed-By: David Carlier <devnexen@gmail.com>

PR-URL: #30374 Refs: nodejs/quic#141 Refs: nodejs/quic#149 Refs: nodejs/quic#141 Refs: nodejs/quic#165 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

This is no longer necessary now that the copyable `BaseObjectPtr` is available (as opposed to the only-movable `v8::Global`). PR-URL: #30374 Refs: nodejs/quic#141 Refs: nodejs/quic#149 Refs: nodejs/quic#141 Refs: nodejs/quic#165 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

This allows keeping `BaseObjectPtr`s to `HandleWrap` instances. Previously, the pointer kept the `HandleWrap` object alive, leaving the Environment cleanup code that waits for the handle list to drain in a busy loop, because only the `HandleWrap` destructor removed the item from the list. Refs: nodejs/quic#165 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> PR-URL: #30374 Refs: nodejs/quic#141 Refs: nodejs/quic#149 Refs: nodejs/quic#141 Reviewed-By: David Carlier <devnexen@gmail.com>

Landed in efce655...1317ac6

@addaleax should this be backported or is this blocked on another PR that should be backported first? It does not land cleanly on our v13.x-staging currently. Please either update the labels accordingly or open a manual backport, thanks!

I think this should have an easier time landing after the V8 7.9 backport.

Referring to `BaseObject` instances using standard C++ smart pointers can interfere with BaseObject’s own cleanup mechanisms (explicit delete, delete-on-GC and delete-on-cleanup). Introducing custom smart pointers allows referring to `BaseObject`s safely while keeping those mechanisms intact. Refs: nodejs/quic#141 Refs: nodejs/quic#149 Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: #30374 Refs: nodejs/quic#165 Reviewed-By: David Carlier <devnexen@gmail.com>

@addaleax ASAN is showing a memory leak apparently originated here (BaseObjectPtrImpl::PointerData is leaking). Several tests are showing this warning. The simples one (less operations) is cctest BaseObjectPtrTest::ScopedDetachedWithWeak. Seems like the weak_ptr_count is not decreased after BaseObjectPtr is deleted (after the inner scope ends). I don't think this is the expected behavior, but I'm not sure. If it is the expected behavior, we might want to ignore LSAN for pointer_data.

For most of these, when the object is freed is dependent entirely on the garbage collector. Could it be the case that the reported leaks are simply objects that the garbage collector hasn't gotten to yet?

Fix the condition for deleting the underlying data pointed to by a `BaseObjectWeakPtr`, which erroneously skipped that deletion when `ptr->get()` was `nullptr`. This fixes a memory leak reported by some of the tests. Refs: nodejs#30374 (comment)

@mmarchini Thanks, see #32393 for a fix. 👍

@jasnell For context, the test referred to above is dealing with “detached” BaseObjects in the sense of

node/src/base_object.h

Lines 95 to 99 in c160073

    
           // Can be used to avoid this object keepling itself alive as a GC root 
        
           // indefinitely, for example when this object is owned and deleted by another 
        
           // BaseObject once that is torn down. This can only be called when there is 
        
           // a BaseObjectPtr to this object. 
        
           inline void Detach();

GC should not come into play as the objects are explicitly owned by the shared pointers in that case.

For most of these, when the object is freed is dependent entirely on the garbage collector. Could it be the case that the reported leaks are simply objects that the garbage collector hasn't gotten to yet?

This test doesn't rely on GC.

#32393 fixed the issue :)

Ah right right.. carry on and don't mind me :-D

Fix the condition for deleting the underlying data pointed to by a `BaseObjectWeakPtr`, which erroneously skipped that deletion when `ptr->get()` was `nullptr`. This fixes a memory leak reported by some of the tests. Refs: nodejs#30374 (comment)

Fix the condition for deleting the underlying data pointed to by a `BaseObjectWeakPtr`, which erroneously skipped that deletion when `ptr->get()` was `nullptr`. This fixes a memory leak reported by some of the tests. Refs: #30374 (comment) PR-URL: #32393 Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

addaleax added 5 commits November 12, 2019 14:32

http2: use custom BaseObject smart pointers

c371a37

Refs: nodejs/quic#141 Reviewed-By: James M Snell <jasnell@gmail.com>

src: use BaseObjectPtr for keeping channel alive in dns bindings

697e719

src: remove keep alive option from SetImmediate()

82f06be

This is no longer necessary now that the copyable `BaseObjectPtr` is available (as opposed to the only-movable `v8::Global`).

addaleax requested a review from jasnell November 12, 2019 14:44

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 12, 2019

jasnell approved these changes Nov 12, 2019

View reviewed changes

devnexen approved these changes Nov 12, 2019

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 12, 2019

addaleax closed this Nov 19, 2019

addaleax deleted the baseobjectptr branch November 19, 2019 13:00

BridgeAR added the backport-requested-v13.x label Nov 19, 2019

addaleax removed the backport-requested-v13.x label Nov 19, 2019

BridgeAR added the backport-blocked-v13.x label Nov 19, 2019

addaleax mentioned this pull request Mar 20, 2020

src: delete BaseObjectWeakPtr data when pointee is gone #32393

Closed

2 tasks

codebytere mentioned this pull request Apr 3, 2020

v12.16.2 proposal #32313

Merged

src: introduce custom smart pointers for `BaseObject`s by addaleax · Pull Request #30374 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: introduce custom smart pointers for BaseObjects - #30374

src: introduce custom smart pointers for BaseObjects#30374addaleax wants to merge 5 commits intonodejs:masternodejs/node:masterfrom addaleax:baseobjectptraddaleax/node:baseobjectptrCopy head branch name to clipboard

Conversation

addaleax commented Nov 12, 2019

src: introduce custom smart pointers for BaseObjects

http2: use custom BaseObject smart pointers

src: use BaseObjectPtr for keeping channel alive in dns bindings

src: remove keep alive option from SetImmediate()

src: remove HandleWrap instances from list once closed

Checklist

Uh oh!

nodejs-github-bot commented Nov 12, 2019

Uh oh!

nodejs-github-bot commented Nov 12, 2019

Uh oh!

nodejs-github-bot commented Nov 13, 2019

Uh oh!

nodejs-github-bot commented Nov 18, 2019

Uh oh!

nodejs-github-bot commented Nov 18, 2019

Uh oh!

addaleax commented Nov 19, 2019

Uh oh!

BridgeAR commented Nov 19, 2019

Uh oh!

addaleax commented Nov 19, 2019

Uh oh!

mmarchini commented Mar 20, 2020

Uh oh!

jasnell commented Mar 20, 2020

Uh oh!

addaleax commented Mar 20, 2020

Uh oh!

mmarchini commented Mar 20, 2020

Uh oh!

jasnell commented Mar 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

src: introduce custom smart pointers for BaseObjects#30374
addaleax wants to merge 5 commits into
nodejs:masterfrom
addaleax:baseobjectptr