Remove StreamBase templating #25142
Closed
Remove StreamBase templating #25142
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
nodejs-github-bot
added
c++
addaleax
reviewed
Dec 19, 2018
src/stream_base-inl.h
Outdated
| #define STREAM_BASE_METHOD(func, str) \ | ||
| do { \ | ||
| Local<FunctionTemplate> func_templ = \ | ||
| env->NewFunctionTemplate(func, \ |
There was a problem hiding this comment.
Btw, we use 4 spaces of indentation here – and maybe it makes sense to turn this macro into a function?
There was a problem hiding this comment.
Tips: use CLANG_FORMAT_START=master make format-cpp to auto format, or replace master with some other commit that you are branching from
|
updated @addaleax @joyeecheung, PTAL |
jasnell
approved these changes
Dec 20, 2018
|
Looks like there is an intermittent timeout issue on some platforms with |
addaleax
added
author ready
|
@maclover7 It looks like the issue is that some streams, in particular libuv streams, delete the associated C++ object before the JS object is garbage collected. They set the first internal field to This would be a (but maybe not the best?) fix: diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h
index 1842962192db..d672fc5a0dae 100644
--- a/src/stream_base-inl.h
+++ b/src/stream_base-inl.h
@@ -274,6 +274,9 @@ inline void StreamBase::AttachToObject(v8::Local<v8::Object> obj) {
}
inline StreamBase* StreamBase::FromObject(v8::Local<v8::Object> obj) {
+ if (obj->GetAlignedPointerFromInternalField(0) == nullptr)
+ return nullptr;
+
return static_cast<StreamBase*>(
obj->GetAlignedPointerFromInternalField(kStreamBaseField));
} |
|
Ping @maclover7 – barring better ideas, I can also apply the patch above and push to this PR for you? |
|
Hm … ping @maclover7 again? :) |
|
I’ve rebased + pushed the suggested change… @jasnell Can you confirm your approval? |
jasnell
approved these changes
Mar 7, 2019
|
Landed in 4697e1b 🎉 |
addaleax
pushed a commit
that referenced
this pull request
Mar 8, 2019
PR-URL: #25142 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax
added a commit
to addaleax/node
that referenced
this pull request
Mar 8, 2019
Since 4697e1b, it is no longer necessary to use `v8::External`s to pass `StreamBase` instances to native functions. Refs: nodejs#25142
3 tasks
danbev
pushed a commit
that referenced
this pull request
Mar 11, 2019
Since 4697e1b, it is no longer necessary to use `v8::External`s to pass `StreamBase` instances to native functions. PR-URL: #26510 Refs: #25142 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR
pushed a commit
to BridgeAR/node
that referenced
this pull request
Mar 12, 2019
PR-URL: nodejs#25142 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR
pushed a commit
that referenced
this pull request
Mar 13, 2019
Since 4697e1b, it is no longer necessary to use `v8::External`s to pass `StreamBase` instances to native functions. PR-URL: #26510 Refs: #25142 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR
pushed a commit
that referenced
this pull request
Mar 14, 2019
PR-URL: #25142 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR
pushed a commit
that referenced
this pull request
Mar 14, 2019
Since 4697e1b, it is no longer necessary to use `v8::External`s to pass `StreamBase` instances to native functions. PR-URL: #26510 Refs: #25142 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
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.
First attempt at trying to remove the templating in shared
StreamBasemethods. This approach casts directly toStreamBasevia an internal field.Unfortunately, I keep hitting linker errors when trying to run
make -j8, and would appreciate any help in figuring out where they're coming from:Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes