src,stream: use SetAccessorProperty instead of SetAccessor #17665

jure · 2017-12-13T22:45:36Z

As per #17636's solution 1 (found in #17636 (comment)), this PR allows Object.getOwnPropertyDescriptor(process.stdin._handle.__proto__, 'bytesRead') to not throw anymore and return an object/property descriptor. However, accessing this property using process.stdin._handle.__proto__.bytesRead will still throw (as is the current case on 8.9.2+).

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

Affected core subsystem(s)

src, stream

Fixes: nodejs#17636 Refs: nodejs#16482 Refs: nodejs#16860

addaleax · 2017-12-13T23:43:49Z

test/parallel/test-stream-base-prototype-accessors.js

+
+  // Should not throw for Object.getOwnPropertyDescriptor
+  assert.strictEqual(
+    typeof (Object.getOwnPropertyDescriptor(TTY.prototype, 'bytesRead')),


You don’t need the parentheses after typeof here

👍 Removed.

addaleax · 2017-12-13T23:45:40Z

CI: https://ci.nodejs.org/job/node-test-commit/14814/

TimothyGu

This PR should also update the following files. In all of them, look for PrototypeTemplate()->SetAccessor to find which ones to replace.

node_crypto.cc
udp_wrap.cc

Ideally, we shouldn't use SetAccessor on InstanceTemplate() either, other than under exceptional circumstances. Here're two files that are easier to fix (look for SetAccessor):

node_crypto.cc
node_perf.cc

TimothyGu · 2017-12-14T00:25:30Z

src/stream_base-inl.h

-                                      signature);
+
+  Local<Signature> signature =
+      Signature::New(env->isolate(), t);


For the person landing this commit: these two lines could be merged into a single line, which might help with readability.

👍 This used to be AccessorSignature, and was over the limit. Fixed.

TimothyGu · 2017-12-14T00:28:05Z

src/stream_base-inl.h

+    env->isolate(),
+    GetFD<Base>,
+    Local<Value>(),
+    signature);


For the person landing this commit: for consistency the indentation should look something like:

Local<FunctionTemplate> get_fd_templ = FunctionTemplate::New(env->isolate(), GetFD<Base>, Local<Value>(), signature);

👍 Fixed, was struggling a bit with how to indent it before.

TimothyGu · 2017-12-14T00:29:59Z

test/parallel/test-stream-base-prototype-accessors.js

 {
-  const msg = /TypeError: Method \w+ called on incompatible receiver/;
+  // Should throw instead of raise assertions
+  const msg = /TypeError: Illegal invocation/;


Even better, change all uses of msg below to TypeError, because the exact error message may differ depending on the JavaScript engine.

👍 Fixed.

TimothyGu

Almost there!

TimothyGu · 2017-12-14T01:22:02Z

src/node_crypto.cc

@@ -544,14 +545,18 @@ void SecureContext::Initialize(Environment* env, Local<Object> target) {
  t->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "kTicketKeyIVIndex"),
         Integer::NewFromUnsigned(env->isolate(), kTicketKeyIVIndex));

-  t->PrototypeTemplate()->SetAccessor(
+  Local<FunctionTemplate> ctx_getter_templ =
+    FunctionTemplate::New(env->isolate(),


We indent by four spaces in line continuations :)

Here and also in other C++ files.

TimothyGu · 2017-12-14T01:28:47Z

test/parallel/test-stream-base-prototype-accessors.js

+  assert.strictEqual(
+    typeof Object.getOwnPropertyDescriptor(TTY.prototype, '_externalStream'),
+    'object'
+  );


Similar tests should exist for Object.getOwnPropertyDescriptor(crypto.SecureContext.prototype, '_external') and Object.getOwnPropertyDescriptor(crypto.Connection.prototype, '_external') where crypto = process.binding('crypto'), and Object.getOwnPropertyDescriptor(process.binding('udp_wrap').UDP.prototype, 'fd').

👍 Added.

TimothyGu · 2017-12-14T01:35:31Z

src/node_crypto.cc

+  Local<FunctionTemplate> ctx_getter_templ =
+    FunctionTemplate::New(env->isolate(),
+                          CtxGetter,
+                          Local<Value>(),


We usually use env->as_external() for the parameter right here, so that we can get access to the Environment object in the callback itself. Again, here and below.

You mean instead of Local<Value>()?

Yes.The same also needs to be done for all other such instances, otherwise I'm fairly sure some tests will fail because of this.

Tests completed fine with just Local<Value>(), so that's a bit worrying, but it's perhaps out of the scope of this PR to add tests for that. I've now switched to env->as_external wherever it was previously used.

jure · 2017-12-14T02:20:37Z

That should be that I think! Fun stuff :)

jure · 2017-12-14T02:33:18Z

src/node_crypto.cc

+  Local<FunctionTemplate> verify_error_getter_templ =
+      FunctionTemplate::New(env->isolate(),
+                            DiffieHellman::VerifyErrorGetter,
+                            Local<Value>(),


Would env->as_external() be appropriate here as well? Or better yet, is there a place where it isn't appropriate (there are 10 places still using it)?

I guess anywhere where as_external was previously used. I'll get on it.

From a quick look: it would be appropriate at all of those places. In fact if you look at the current SetAccessor()-based code, all of them have env->as_external() for the data parameter.

TimothyGu

LGTM. The comment is optional.

TimothyGu · 2017-12-14T02:56:44Z

src/node_perf.cc

+  Local<FunctionTemplate> get_performance_entry_name_templ =
+      FunctionTemplate::New(env->isolate(),
+                            GetPerformanceEntryName,
+                            Local<Value>(),


I know they didn't use to get env->as_external(), so this should be fine. But for sake of consistency I'd rather still have env->as_external() here.

Sure, that makes sense. Done 👍

TimothyGu · 2017-12-14T02:57:21Z

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

jure · 2017-12-14T03:34:15Z

What's with the failures on CI (Windows, Linux and SmartOS)? How does one look into a failed build?

TimothyGu · 2017-12-14T05:29:35Z

The CI failures all seem to be infrastructural. I'll start a rebuild to see if the situation improves. If not we can land this as-is.

CI: https://ci.nodejs.org/job/node-test-commit/14820/

apapirovski · 2017-12-15T14:25:53Z

Thank you @jure! This is an awesome 1st PR! 👍

jure · 2017-12-15T16:12:13Z

Thanks @apapirovski :) Let me know if there's anything else. I've already put a bottle of bubbly into the fridge, just waiting for that merge notification!

TimothyGu · 2017-12-15T16:54:06Z

@jure We usually tend to wait a few days before applying to make sure everyone who wants to take a look at this PR got a chance to. This should get applied in a few hours.

PR-URL: #17665 Fixes: #17636 Refs: #16482 Refs: #16860 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

apapirovski · 2017-12-17T18:09:40Z

Landed in efffcc2. Thanks @jure and congrats on becoming a Contributor!! 🎉

(Time to pop that bubbly!)

PR-URL: #17665 Fixes: #17636 Refs: #16482 Refs: #16860 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

PR-URL: nodejs/node#17665 Fixes: nodejs/node#17636 Refs: nodejs/node#16482 Refs: nodejs/node#16860 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins · 2018-01-23T07:06:48Z

Do we want to land this on LTS? It lands cleanly on v8.x and v6.x, just unsure if we should wait a bit before landing

TimothyGu · 2018-01-23T15:46:34Z

@MylesBorins I’m +1 on backporting this.

MylesBorins · 2018-02-27T03:20:31Z

This does not land cleanly on v6.x due to the performance API not existing on that release stream. Does it still make sense to backport?

edit: landing on v8.x also broke a test, need a backport to both branches

=== release test-tls-external-accessor ===
Path: parallel/test-tls-external-accessor
assert.js:649
      throw actual;
      ^

TypeError: Illegal invocation
    at assert.throws (/Users/mborins/code/node/v8.x/test/parallel/test-tls-external-accessor.js:14:28)
    at tryBlock (assert.js:619:5)
    at innerThrows (assert.js:638:18)
    at Function.throws (assert.js:662:3)
    at Object.<anonymous> (/Users/mborins/code/node/v8.x/test/parallel/test-tls-external-accessor.js:14:10)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
Command: out/Release/node /Users/mborins/code/node/v8.x/test/parallel/test-tls-external-accessor.js

PR-URL: nodejs#17665 Fixes: nodejs#17636 Refs: nodejs#16482 Refs: nodejs#16860 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Backport-PR-URL: #20456 PR-URL: #17665 Fixes: #17636 Refs: #16482 Refs: #16860 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

PR-URL: nodejs/node#17665 Fixes: nodejs/node#17636 Refs: nodejs/node#16482 Refs: nodejs/node#16860 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

* src: replace SetAccessor w/ SetAccessorProperty PR-URL: nodejs/node#17665 Fixes: nodejs/node#17636 Refs: nodejs/node#16482 Refs: nodejs/node#16860 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> * test: make test-tls-external-accessor agnostic Remove reliance on V8-specific error messages in test/parallel/test-tls-external-accessor.js. Check that the error is a `TypeError`. The test should now be successful without modification using ChakraCore. Backport-PR-URL: nodejs/node#20456 PR-URL: nodejs/node#16272 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Change to using SetAccessorProperty instead of SetAccessor in StreamBase

329e162

Fixes: nodejs#17636 Refs: nodejs#16482 Refs: nodejs#16860

jure changed the title ~~Change to using SetAccessorProperty instead of SetAccessor in StreamBase~~ src,stream: use SetAccessorProperty instead of SetAccessor in StreamBase Dec 13, 2017

addaleax approved these changes Dec 13, 2017

View reviewed changes

addaleax 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 Dec 13, 2017

Remove extra parentheses

dcca3a5

TimothyGu reviewed Dec 14, 2017

View reviewed changes

jure added 3 commits December 14, 2017 00:52

Fix indentation

ebbe147

Check for TypeError instead of matching error msg

a03028f

Change PrototypeTemplate SetAccessor to SetAccessorProperty

06d8157

jure changed the title ~~src,stream: use SetAccessorProperty instead of SetAccessor in StreamBase~~ src,stream: use SetAccessorProperty instead of SetAccessor Dec 14, 2017

Changing SetAccessor to SetAccessorProperty in udp_wrap

0b96b12

TimothyGu reviewed Dec 14, 2017

View reviewed changes

Change InstanceTemplate SetAccessor to SetAccessorProperty

26ffff1

TimothyGu reviewed Dec 14, 2017

View reviewed changes

jure added 5 commits December 14, 2017 01:44

Change InstanceTemplate SetAccessor to SetAccessorProperty

9292fa0

Fix indentation and linting errors in node_crypto

354008a

Fixes for indentation

a199e07

Add more tests for UDP and crypto

5eb6507

Fixes for indentation

232d2be

jure commented Dec 14, 2017

View reviewed changes

jure added 2 commits December 14, 2017 02:40

Use env->as_external to get access to Environment in callback

d89578a

More env->as_external to get access to Environment in callback

5a60daa

TimothyGu approved these changes Dec 14, 2017

View reviewed changes

Use env->as_external in node_perf too

84905d6

apapirovski closed this Dec 17, 2017

TimothyGu mentioned this pull request Jan 4, 2018

Getting ._handle through --inspect with chrome developer tools crashes Node.js #16949

Closed

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added lts-watch-v6.x labels Jan 23, 2018

MylesBorins added backport-requested-v6.x and removed lts-watch-v6.x labels Feb 27, 2018

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

jasonrudolph mentioned this pull request Jun 14, 2018

[Atom-1.28.0-beta1][OSX] Mocha tests fail in headless mode, but not in headed mode atom/atom#17409

Closed

1 task

src,stream: use SetAccessorProperty instead of SetAccessor #17665

src,stream: use SetAccessorProperty instead of SetAccessor #17665

Conversation

jure commented Dec 13, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Dec 13, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

TimothyGu Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

TimothyGu Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jure Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

TimothyGu Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jure commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Dec 14, 2017

jure commented Dec 14, 2017

TimothyGu commented Dec 14, 2017

apapirovski commented Dec 15, 2017

jure commented Dec 15, 2017

TimothyGu commented Dec 15, 2017

apapirovski commented Dec 17, 2017 • edited Loading

MylesBorins commented Jan 23, 2018

TimothyGu commented Jan 23, 2018

MylesBorins commented Feb 27, 2018 • edited Loading

jure commented Dec 13, 2017 •

edited

Loading

TimothyGu Dec 14, 2017 •

edited

Loading

TimothyGu Dec 14, 2017 •

edited

Loading

jure Dec 14, 2017 •

edited

Loading

TimothyGu Dec 14, 2017 •

edited

Loading

TimothyGu Dec 14, 2017 •

edited

Loading

apapirovski commented Dec 17, 2017 •

edited

Loading

MylesBorins commented Feb 27, 2018 •

edited

Loading