src: refactor BaseObject internal field management
#20455
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Instead of storing a pointer whose type refers to the specific subclass of `BaseObject`, just store a `BaseObject*` directly. This means in particular that one can cast to classes along the way of the inheritance chain without issues, and that `BaseObject*` no longer needs to be the first superclass in the case of multiple inheritance. In particular, this renders hack-y solutions to this problem (like ddc19be) obsolete and addresses a `TODO` comment of mine. - Move wrapping/unwrapping methods to the `BaseObject` class. We use these almost exclusively for `BaseObject`s, and I hope that this gives a better idea of how (and for what) these are used in our code. - Perform initialization/deinitialization of the internal field in the `BaseObject*` constructor/destructor. This makes the code a bit more obviously correct, avoids explicit calls for this in subclass constructors, and in particular allows us to avoid crash situations when we previously called `ClearWrap()` during GC. This also means that we enforce that the object passed to the `BaseObject` constructor needs to have an internal field. This is the only reason for the test change. - Change the signature of `MakeWeak()` to not require a pointer argument. Previously, this would always have been the same as `this`, and no other value made sense. Also, the parameter was something that I personally found somewhat confusing when becoming familiar with Node’s code. - Add a `TODO` comment that motivates switching to real inheritance for the JS types we expose from the native side. This patch brings us a lot closer to being able to do that. - Some less significant drive-by cleanup. Since we *effectively* already store the `BaseObject*` pointer anyway since ddc19be, I do not think that this is going to have any impact on diagnostic tooling. Fixes: nodejs#18897
nodejs-github-bot
added
c++
|
I opened nodejs/build#1260 to track the windows failure, I'm seeing the same thing in my CI runs. |
devsnek
approved these changes
May 1, 2018
|
FYI: the Windows CI issue should be resolved now. |
bnoordhuis
approved these changes
May 1, 2018
src/base_object-inl.h
Outdated
| Wrap(handle, ptr); | ||
| persistent_handle_.SetWeak<Type>(ptr, WeakCallback<Type>, | ||
| v8::WeakCallbackType::kParameter); | ||
| template<typename T> |
There was a problem hiding this comment.
Space before <. (Also in base_object.h.)
There was a problem hiding this comment.
Ugh. I need to add this to the linter some day. 😄 Done!
|
|
||
| #define ASSIGN_OR_RETURN_UNWRAP(ptr, obj, ...) \ | ||
| do { \ | ||
| *ptr = static_cast<typename std::remove_reference<decltype(*ptr)>::type>( \ |
There was a problem hiding this comment.
Can you #include <type_traits> in this file and remove it from util.h?
|
Once more CI: https://ci.nodejs.org/job/node-test-commit/18169/ |
danbev
approved these changes
May 2, 2018
addaleax
added
the
author ready
addaleax
removed
the
author ready
addaleax
added a commit
that referenced
this pull request
May 3, 2018
- Instead of storing a pointer whose type refers to the specific subclass of `BaseObject`, just store a `BaseObject*` directly. This means in particular that one can cast to classes along the way of the inheritance chain without issues, and that `BaseObject*` no longer needs to be the first superclass in the case of multiple inheritance. In particular, this renders hack-y solutions to this problem (like ddc19be) obsolete and addresses a `TODO` comment of mine. - Move wrapping/unwrapping methods to the `BaseObject` class. We use these almost exclusively for `BaseObject`s, and I hope that this gives a better idea of how (and for what) these are used in our code. - Perform initialization/deinitialization of the internal field in the `BaseObject*` constructor/destructor. This makes the code a bit more obviously correct, avoids explicit calls for this in subclass constructors, and in particular allows us to avoid crash situations when we previously called `ClearWrap()` during GC. This also means that we enforce that the object passed to the `BaseObject` constructor needs to have an internal field. This is the only reason for the test change. - Change the signature of `MakeWeak()` to not require a pointer argument. Previously, this would always have been the same as `this`, and no other value made sense. Also, the parameter was something that I personally found somewhat confusing when becoming familiar with Node’s code. - Add a `TODO` comment that motivates switching to real inheritance for the JS types we expose from the native side. This patch brings us a lot closer to being able to do that. - Some less significant drive-by cleanup. Since we *effectively* already store the `BaseObject*` pointer anyway since ddc19be, I do not think that this is going to have any impact on diagnostic tooling. Fixes: #18897 PR-URL: #20455 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
MylesBorins
pushed a commit
that referenced
this pull request
May 4, 2018
- Instead of storing a pointer whose type refers to the specific subclass of `BaseObject`, just store a `BaseObject*` directly. This means in particular that one can cast to classes along the way of the inheritance chain without issues, and that `BaseObject*` no longer needs to be the first superclass in the case of multiple inheritance. In particular, this renders hack-y solutions to this problem (like ddc19be) obsolete and addresses a `TODO` comment of mine. - Move wrapping/unwrapping methods to the `BaseObject` class. We use these almost exclusively for `BaseObject`s, and I hope that this gives a better idea of how (and for what) these are used in our code. - Perform initialization/deinitialization of the internal field in the `BaseObject*` constructor/destructor. This makes the code a bit more obviously correct, avoids explicit calls for this in subclass constructors, and in particular allows us to avoid crash situations when we previously called `ClearWrap()` during GC. This also means that we enforce that the object passed to the `BaseObject` constructor needs to have an internal field. This is the only reason for the test change. - Change the signature of `MakeWeak()` to not require a pointer argument. Previously, this would always have been the same as `this`, and no other value made sense. Also, the parameter was something that I personally found somewhat confusing when becoming familiar with Node’s code. - Add a `TODO` comment that motivates switching to real inheritance for the JS types we expose from the native side. This patch brings us a lot closer to being able to do that. - Some less significant drive-by cleanup. Since we *effectively* already store the `BaseObject*` pointer anyway since ddc19be, I do not think that this is going to have any impact on diagnostic tooling. Fixes: #18897 PR-URL: #20455 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Merged
MylesBorins
pushed a commit
that referenced
this pull request
May 8, 2018
- Instead of storing a pointer whose type refers to the specific subclass of `BaseObject`, just store a `BaseObject*` directly. This means in particular that one can cast to classes along the way of the inheritance chain without issues, and that `BaseObject*` no longer needs to be the first superclass in the case of multiple inheritance. In particular, this renders hack-y solutions to this problem (like ddc19be) obsolete and addresses a `TODO` comment of mine. - Move wrapping/unwrapping methods to the `BaseObject` class. We use these almost exclusively for `BaseObject`s, and I hope that this gives a better idea of how (and for what) these are used in our code. - Perform initialization/deinitialization of the internal field in the `BaseObject*` constructor/destructor. This makes the code a bit more obviously correct, avoids explicit calls for this in subclass constructors, and in particular allows us to avoid crash situations when we previously called `ClearWrap()` during GC. This also means that we enforce that the object passed to the `BaseObject` constructor needs to have an internal field. This is the only reason for the test change. - Change the signature of `MakeWeak()` to not require a pointer argument. Previously, this would always have been the same as `this`, and no other value made sense. Also, the parameter was something that I personally found somewhat confusing when becoming familiar with Node’s code. - Add a `TODO` comment that motivates switching to real inheritance for the JS types we expose from the native side. This patch brings us a lot closer to being able to do that. - Some less significant drive-by cleanup. Since we *effectively* already store the `BaseObject*` pointer anyway since ddc19be, I do not think that this is going to have any impact on diagnostic tooling. Fixes: #18897 PR-URL: #20455 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
3 tasks
addaleax
added a commit
to addaleax/node
that referenced
this pull request
Jul 3, 2018
An oversight in an earlier commit led to a memory leak in the untypical situation that zlib instances are created but never used, because zlib handles no longer started out their life as weak handles. The bug was introduced in bd20110. Refs: nodejs#20455 PR-URL: nodejs#21607 Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos
pushed a commit
that referenced
this pull request
Jul 4, 2018
An oversight in an earlier commit led to a memory leak in the untypical situation that zlib instances are created but never used, because zlib handles no longer started out their life as weak handles. The bug was introduced in bd20110. Refs: #20455 PR-URL: #21607 Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
src: access
ContextifyContext*more directly in property cbssrc: refactor
BaseObjectinternal field managementInstead of storing a pointer whose type refers to the specific
subclass of
BaseObject, just store aBaseObject*directly.This means in particular that one can cast to classes along
the way of the inheritance chain without issues, and that
BaseObject*no longer needs to be the first superclassin the case of multiple inheritance.
In particular, this renders hack-y solutions to this problem (like
ddc19be) obsolete and addresses
a
TODOcomment of mine.Move wrapping/unwrapping methods to the
BaseObjectclass.We use these almost exclusively for
BaseObjects, and I hopethat this gives a better idea of how (and for what) these are used
in our code.
Perform initialization/deinitialization of the internal field
in the
BaseObject*constructor/destructor. This makes the codea bit more obviously correct, avoids explicit calls for this
in subclass constructors, and in particular allows us to avoid
crash situations when we previously called
ClearWrap()during GC.
This also means that we enforce that the object passed to the
BaseObjectconstructor needs to have an internal field.This is the only reason for the test change.
Change the signature of
MakeWeak()to not require a pointerargument. Previously, this would always have been the same
as
this, and no other value made sense. Also, the parameterwas something that I personally found somewhat confusing
when becoming familiar with Node’s code.
Add a
TODOcomment that motivates switching to real inheritancefor the JS types we expose from the native side. This patch
brings us a lot closer to being able to do that.
Some less significant drive-by cleanup.
Since we effectively already store the
BaseObject*pointeranyway since ddc19be, I do not
think that this is going to have any impact on diagnostic tooling.
Fixes: src: get rid of ClearWrap() #18897
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes