util: correctly inspect Map/Set Iterators #3119

evanlucas · 2015-09-29T19:34:46Z

Currently, inspecting new Map().keys() or other iterators will return an empty object.

I am opening more for discussion of how we want (if at all) to address not being able to inspect map and set iterators. I first had first attempting using Debug.MakeMirror but the performance of doing it this way was about 10x better.

Related: #3107

vkurchatkin · 2015-09-29T20:40:30Z

lib/util.js

@@ -486,6 +494,16 @@ function formatMap(ctx, value, recurseTimes, visibleKeys, keys) {
  return output;
 }

+function formatIterator(ctx, value, recurseTimes, visibleKeys, keys) {
+  var output = [];
+  for (let o of value) {


This will consume iterator entirely and make it useless

For Maps and Sets?

If you print an iterator, python just prints the type of iterator. May be we should do the same. Otherwise we'll exhaust the iterator as vkurchatkin pointed out.

Ah yea, I see what you mean.

So I guess that means that we will have to use Debug.MakeMirror and fetch the values?

thefourtheye · 2015-09-30T03:05:38Z

src/node_util.cc

+static void IsMapOrSetIterator(const FunctionCallbackInfo<Value>& args) {
+  CHECK_EQ(1, args.Length());
+  args.GetReturnValue().Set(args[0]->IsSetIterator()
+                            || args[0]->IsMapIterator());


Style nit. Or operator should be in the previous line.

@thefourtheye Do we have an eslint rule for that, btw?

make lint passes with both. Be happy to change if that is the preferred way though

bnoordhuis · 2015-09-30T11:19:40Z

src/node_util.cc

+static void IsMapOrSetIterator(const FunctionCallbackInfo<Value>& args) {
+  CHECK_EQ(1, args.Length());
+  args.GetReturnValue().Set(args[0]->IsSetIterator() ||
+                            args[0]->IsMapIterator());


Doesn't it fit on one line? It works out to exactly 80 characters, doesn't it?

evanlucas · 2015-09-30T12:51:28Z

Updated to use Debug.MakeMirror. It also takes advantage of using IteratorMirror.preview which does not change the backing iterator state.

bnoordhuis · 2015-10-01T19:38:55Z

lib/util.js

@@ -486,6 +502,18 @@ function formatMap(ctx, value, recurseTimes, visibleKeys, keys) {
  return output;
 }

+function formatCollectionIterator(ctx, value, recurseTimes, visibleKeys, keys) {
+  Debug = Debug || require('vm').runInDebugContext('Debug');


Can you rebase and use ensureDebugIsInitialized() here?

evanlucas · 2015-10-05T21:01:31Z

Updated to utilize v8::Value::IsMapIterator and v8::Value::IsSetIterator

bnoordhuis · 2015-10-05T21:04:49Z

lib/util.js

@@ -3,6 +3,7 @@
 const uv = process.binding('uv');
 const Buffer = require('buffer').Buffer;
 const internalUtil = require('internal/util');
+const Util = process.binding('util');


Can you name this binding? That's more in line with other source files.

bnoordhuis · 2015-10-05T21:08:00Z

Mostly LGTM. I think the constructor.name === 'Array' check should come with regression tests of its own.

evanlucas · 2015-10-05T21:39:55Z

Alright, broke out the constructor.name === 'Array' check into it's own commit with a test. Is that single test sufficient or should I add more? Honestly, the only other way I've been able to reproduce it is from the map/set iterators

evanlucas · 2015-10-05T21:41:35Z

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

bnoordhuis · 2015-10-05T21:46:58Z

LGTM

In the event an Array is created in a Debug context, the constructor will be Array, but !== Array. This adds a check constructor.name === 'Array' to handle edge cases like that. PR-URL: nodejs#3119 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Previously, a MapIterator or SetIterator would not be inspected properly. This change makes it possible to inspect them by creating a Debug Mirror and previewing the iterators to not consume the actual iterator that we are trying to inspect. This change also adds a node_util binding that uses v8's Value::IsSetIterator and Value::IsMapIterator to verify that the values passed in are actual iterators. Fixes: nodejs#3107 PR-URL: nodejs#3119 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

evanlucas · 2015-10-06T01:29:14Z

Thanks! Landed in 089d688 and 8853388

In the event an Array is created in a Debug context, the constructor will be Array, but !== Array. This adds a check constructor.name === 'Array' to handle edge cases like that. PR-URL: #3119 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Previously, a MapIterator or SetIterator would not be inspected properly. This change makes it possible to inspect them by creating a Debug Mirror and previewing the iterators to not consume the actual iterator that we are trying to inspect. This change also adds a node_util binding that uses v8's Value::IsSetIterator and Value::IsMapIterator to verify that the values passed in are actual iterators. Fixes: #3107 PR-URL: #3119 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Fishrock123 added the util Issues and PRs related to the built-in util module. label Sep 29, 2015

vkurchatkin reviewed Sep 29, 2015
View reviewed changes

evanlucas force-pushed the iterators branch from ed5c500 to 04b8de3 Compare September 30, 2015 03:03

thefourtheye reviewed Sep 30, 2015
View reviewed changes

evanlucas force-pushed the iterators branch from 04b8de3 to 7da3f9a Compare September 30, 2015 03:10

bnoordhuis reviewed Sep 30, 2015
View reviewed changes

targos mentioned this pull request Sep 30, 2015

Speed up promise introspection #3130

Merged

evanlucas force-pushed the iterators branch from 7da3f9a to 8204269 Compare September 30, 2015 12:49

bnoordhuis reviewed Oct 1, 2015
View reviewed changes

evanlucas force-pushed the iterators branch from 8204269 to 2b8c663 Compare October 1, 2015 20:25

evanlucas mentioned this pull request Oct 5, 2015

Time to play with V8 4.6 #2688

Closed

bnoordhuis reviewed Oct 5, 2015
View reviewed changes

evanlucas force-pushed the iterators branch 3 times, most recently from 66169e3 to c7d7eab Compare October 5, 2015 21:38

evanlucas force-pushed the iterators branch from c7d7eab to 0541eee Compare October 6, 2015 01:23

evanlucas added 2 commits October 5, 2015 20:24

util: fix check for Array constructor

089d688

In the event an Array is created in a Debug context, the constructor will be Array, but !== Array. This adds a check constructor.name === 'Array' to handle edge cases like that. PR-URL: nodejs#3119 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

evanlucas force-pushed the iterators branch from 0541eee to 8853388 Compare October 6, 2015 01:28

evanlucas closed this Oct 6, 2015

evanlucas deleted the iterators branch October 6, 2015 01:28

evanlucas merged commit 8853388 into nodejs:master Oct 6, 2015

evanlucas mentioned this pull request Oct 6, 2015

util: fix for inspecting promises #3197

Merged

jasnell mentioned this pull request Oct 8, 2015

Preparing v4.x for LTS #3252

Closed

29 tasks

jasnell added the lts-landed-on-v4.x label Oct 8, 2015

jasnell mentioned this pull request Oct 8, 2015

Release proposal: v4.2.0 'Argon' (LTS) #3258

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: correctly inspect Map/Set Iterators #3119

util: correctly inspect Map/Set Iterators #3119

evanlucas commented Sep 29, 2015

vkurchatkin Sep 29, 2015

evanlucas Sep 30, 2015

thefourtheye Sep 30, 2015

evanlucas Sep 30, 2015

evanlucas Sep 30, 2015

bnoordhuis Sep 30, 2015

thefourtheye Sep 30, 2015

ChALkeR Sep 30, 2015

evanlucas Sep 30, 2015

bnoordhuis Sep 30, 2015

evanlucas commented Sep 30, 2015

bnoordhuis Oct 1, 2015

evanlucas commented Oct 5, 2015

bnoordhuis Oct 5, 2015

bnoordhuis commented Oct 5, 2015

evanlucas commented Oct 5, 2015

evanlucas commented Oct 5, 2015

bnoordhuis commented Oct 5, 2015

evanlucas commented Oct 6, 2015

util: correctly inspect Map/Set Iterators #3119

util: correctly inspect Map/Set Iterators #3119

Conversation

evanlucas commented Sep 29, 2015

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanlucas commented Sep 30, 2015

Choose a reason for hiding this comment

evanlucas commented Oct 5, 2015

Choose a reason for hiding this comment

bnoordhuis commented Oct 5, 2015

evanlucas commented Oct 5, 2015

evanlucas commented Oct 5, 2015

bnoordhuis commented Oct 5, 2015

evanlucas commented Oct 6, 2015