src,quic: add custom smart pointer for BaseObjects #141

addaleax · 2019-09-29T17:51:23Z

This is in response to conversation with @jasnell; the previous approach taken in the QUIC codebase, relying mostly on std::shared_ptr and std::unique_ptr, conflicts with BaseObject’s own lifetime management. In order to avoid these conflicts, this introduces a new type of smart pointer to Node.js specifically for managing and observing the lifetime of BaseObject subclass instances.

The first two commits are from node/master, included so that semantic conflicts with them can be avoided early. These will go away once we rebase.

What also came out of that conversation is that, whether we take this approach or not, some documentation on the BaseObject lifetime management (and the Environment cleanup functionality) is necessary. I don’t think it should hold up this PR, but it would make a good follow-up, especially given that this PR does modify the semantics there and such documentation would look different depending on whether this PR lands.

Main commit:

src: introduce custom smart pointers for `BaseObject`s

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.

Checklist

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

This simplifies the implementation of ELDHistogram a bit, and more generally allows us to have weak JS references associated with `HandleWrap`s. PR-URL: nodejs/node#29317 Reviewed-By: James M Snell <[email protected]>

In 0af62aa, this was overlooked, with it possibly leading to hard crashes. Refs: nodejs/node#29317 PR-URL: nodejs/node#29640 Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Rich Trott <[email protected]>

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.

Previously, the QuicSocket instance would always be destroyed first during cleanup (because it is a `HandleWrap`), destroying the other native objects associated with it in the process. This makes sure that cleanup also works when the session is detached from the “real” network socket. In particular, this is useful for the future possibility of fully detaching sessions from sockets.

addaleax · 2019-09-29T18:50:38Z

src/base_object.h

  Environment* env_;
+  PointerData* pointer_data_ = nullptr;


Btw, I’ve considered merging the two members into one tagged pointer (Environment* by default, PointerData* otherwise + an Environment* member in PointerData) in order to avoid the extra memory overhead for most BaseObjects, but playing around with similar code and looking at the compiler output I got worried about the added CPU cost, and so I let that be for now.

jasnell

Very nice

jasnell · 2019-09-30T16:35:15Z

@addaleax ... with regards to the two upstream commits, should I go ahead and land this now or pull in the latest round of updates from nodejs/node then drop the extra commits from this PR?

addaleax · 2019-10-01T13:23:21Z

@jasnell I’ve pushed another commit with the move constructors suggested in the TODO comment and more tests. (Just so you’re aware as the sole reviewer so far.)

@addaleax ... with regards to the two upstream commits, should I go ahead and land this now or pull in the latest round of updates from nodejs/node then drop the extra commits from this PR?

I don’t think it makes much of a difference – the commits should be dropped automatically once we rebase. Feel free to go with what’s easier for you :)

jasnell · 2019-10-01T14:58:42Z

Ok, I'll land this first a bit later on today then do a rebase and force update to master by the end of the day.

jasnell · 2019-10-01T20:21:16Z

Landed!

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. PR-URL: #141 Reviewed-By: James M Snell <[email protected]>

PR-URL: #141 Reviewed-By: James M Snell <[email protected]>

Previously, the QuicSocket instance would always be destroyed first during cleanup (because it is a `HandleWrap`), destroying the other native objects associated with it in the process. This makes sure that cleanup also works when the session is detached from the “real” network socket. In particular, this is useful for the future possibility of fully detaching sessions from sockets. PR-URL: #141 Reviewed-By: James M Snell <[email protected]>

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. PR-URL: #141 Reviewed-By: James M Snell <[email protected]>

PR-URL: #141 Reviewed-By: James M Snell <[email protected]>

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. PR-URL: #141 Reviewed-By: James M Snell <[email protected]>

PR-URL: #141 Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>

Refs: nodejs/quic#141 Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> PR-URL: nodejs#30374 Refs: nodejs/quic#165 Reviewed-By: David Carlier <[email protected]>

Refs: nodejs/quic#141 Reviewed-By: James M Snell <[email protected]> PR-URL: nodejs#30374 Refs: nodejs/quic#149 Refs: nodejs/quic#165 Reviewed-By: David Carlier <[email protected]>

PR-URL: nodejs#30374 Refs: nodejs/quic#141 Refs: nodejs/quic#149 Refs: nodejs/quic#141 Refs: nodejs/quic#165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: David Carlier <[email protected]>

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

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 <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> PR-URL: nodejs#30374 Refs: nodejs/quic#141 Refs: nodejs/quic#149 Refs: nodejs/quic#141 Reviewed-By: David Carlier <[email protected]>

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 <[email protected]> Backport-PR-URL: #32301 PR-URL: #30374 Refs: nodejs/quic#165 Reviewed-By: David Carlier <[email protected]>

Refs: nodejs/quic#141 Reviewed-By: James M Snell <[email protected]> Backport-PR-URL: #32301 PR-URL: #30374 Refs: nodejs/quic#149 Refs: nodejs/quic#165 Reviewed-By: David Carlier <[email protected]>

Backport-PR-URL: #32301 PR-URL: #30374 Refs: nodejs/quic#141 Refs: nodejs/quic#149 Refs: nodejs/quic#141 Refs: nodejs/quic#165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: David Carlier <[email protected]>

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

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 <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Backport-PR-URL: #32301 PR-URL: #30374 Refs: nodejs/quic#141 Refs: nodejs/quic#149 Refs: nodejs/quic#141 Reviewed-By: David Carlier <[email protected]>

addaleax added 7 commits September 29, 2019 19:12

src: make ELDHistogram a HandleWrap

f787f88

This simplifies the implementation of ELDHistogram a bit, and more generally allows us to have weak JS references associated with `HandleWrap`s. PR-URL: nodejs/node#29317 Reviewed-By: James M Snell <[email protected]>

src: fix closing weak HandleWraps on GC

1af3703

In 0af62aa, this was overlooked, with it possibly leading to hard crashes. Refs: nodejs/node#29317 PR-URL: nodejs/node#29640 Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Rich Trott <[email protected]>

http2: use custom BaseObject smart pointers

9a3f0dd

quic: use custom BaseObject smart pointers

6eccf46

src: use custom smart pointers for HistogramBase

1428c19

addaleax commented Sep 29, 2019

View reviewed changes

jasnell approved these changes Sep 30, 2019

View reviewed changes

fixup! src: introduce custom smart pointers for BaseObjects

2909021

addaleax mentioned this pull request Oct 1, 2019

TODO: Need to fill out memory tracking #59

Closed

fixup! quic: use custom BaseObject smart pointers

d0b63ad

jasnell closed this Oct 1, 2019

jasnell pushed a commit that referenced this pull request Oct 1, 2019

http2: use custom BaseObject smart pointers

c588393

PR-URL: #141 Reviewed-By: James M Snell <[email protected]>

jasnell pushed a commit that referenced this pull request Oct 1, 2019

quic: use custom BaseObject smart pointers

2c1cdd9

PR-URL: #141 Reviewed-By: James M Snell <[email protected]>

jasnell pushed a commit that referenced this pull request Oct 1, 2019

src: use custom smart pointers for HistogramBase

94b4995

PR-URL: #141 Reviewed-By: James M Snell <[email protected]>

addaleax deleted the teardown-fixes-2 branch October 1, 2019 20:23

jasnell pushed a commit that referenced this pull request Oct 2, 2019

http2: use custom BaseObject smart pointers

4bfbf5d

PR-URL: #141 Reviewed-By: James M Snell <[email protected]>

jasnell pushed a commit that referenced this pull request Oct 3, 2019

http2: use custom BaseObject smart pointers

ba1c9a3

PR-URL: #141 Reviewed-By: James M Snell <[email protected]>

addaleax added a commit to addaleax/node that referenced this pull request Nov 12, 2019

http2: use custom BaseObject smart pointers

c371a37

Refs: nodejs/quic#141 Reviewed-By: James M Snell <[email protected]>

addaleax mentioned this pull request Nov 12, 2019

src: introduce custom smart pointers for BaseObjects nodejs/node#30374

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src,quic: add custom smart pointer for BaseObjects #141

src,quic: add custom smart pointer for BaseObjects #141

addaleax commented Sep 29, 2019

addaleax Sep 29, 2019

jasnell left a comment

jasnell commented Sep 30, 2019

addaleax commented Oct 1, 2019

jasnell commented Oct 1, 2019

jasnell commented Oct 1, 2019

src,quic: add custom smart pointer for BaseObjects #141

src,quic: add custom smart pointer for BaseObjects #141

Conversation

addaleax commented Sep 29, 2019

src: introduce custom smart pointers for BaseObjects

Checklist

addaleax Sep 29, 2019

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

jasnell commented Sep 30, 2019

addaleax commented Oct 1, 2019

jasnell commented Oct 1, 2019

jasnell commented Oct 1, 2019

src: introduce custom smart pointers for `BaseObject`s