Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assert.deepEqual loops forever with circular refs #6416

Closed
kuraga opened this issue Apr 27, 2016 · 5 comments
Closed

assert.deepEqual loops forever with circular refs #6416

kuraga opened this issue Apr 27, 2016 · 5 comments
Labels
assert Issues and PRs related to the assert subsystem. confirmed-bug Issues with confirmed bugs.

Comments

@kuraga
Copy link

kuraga commented Apr 27, 2016

Version: v4.4.3

Platform: Linux 3.13.0-24-generic #47-Ubuntu SMP Fri May 2 23:30:00 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux

Code:

var assert = require('assert');

var b = {};
b.b = b;

var c = {};
c.b = c;

assert.deepEqual(b, c);

Expected: no exceptions.

Actual: RangeError: Maximum call stack size exceeded

See also:

@addaleax addaleax added the assert Issues and PRs related to the assert subsystem. label Apr 27, 2016
@Trott
Copy link
Member

Trott commented Apr 27, 2016

Also affects deepStrictEqual().

@Trott
Copy link
Member

Trott commented Apr 27, 2016

Judging from the test, this is sort of by design. The original behavior was (apparently) to loop forever but now it throws an error instead.

Should be fixable by keeping track of seen references. Not sure about the semver implications or if the Locked status of the API is an issue here or not. (I would consider this a bugfix myself, but others may disagree.)

@addaleax
Copy link
Member

@Trott I’ve looked a bit at the original issue linked above, and I think Ryan Dahl actually just forgot to pull in the patch which does what you suggest (nodejs/node-v0.x-archive#207 (comment)). Would say it’s a bugfix, too.

@Trott
Copy link
Member

Trott commented Apr 27, 2016

@addaleax Cool! I think I've got a fix almost ready to go, but if you're already working on it, I'm happy to drop it and go do something else.

@addaleax
Copy link
Member

@Trott No, sorry, didn’t mean to imply that I was coding anything right now – go ahead! 😄

Trott added a commit to Trott/io.js that referenced this issue Apr 27, 2016
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

Refs: strager/node@56dbd80
Fixes: nodejs#6416
Trott added a commit to Trott/io.js that referenced this issue Apr 28, 2016
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

Fixes: nodejs#6416
@Fishrock123 Fishrock123 added the confirmed-bug Issues with confirmed bugs. label Apr 29, 2016
@Trott Trott closed this as completed in d3aafd0 Apr 29, 2016
Fishrock123 pushed a commit that referenced this issue May 4, 2016
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

PR-URL: #6432
Fixes: #6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
joelostrowski pushed a commit to joelostrowski/node that referenced this issue May 4, 2016
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

PR-URL: nodejs#6432
Fixes: nodejs#6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
azu added a commit to azu/universal-deep-strict-equal that referenced this issue May 6, 2016
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

fix: twada#3
refs:nodejs/node#6416
       nodejs/node#6432
azu added a commit to azu/universal-deep-strict-equal that referenced this issue May 6, 2016
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

fix: twada#3
refs:nodejs/node#6416
       nodejs/node#6432
MylesBorins pushed a commit that referenced this issue Jul 11, 2016
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

PR-URL: #6432
Fixes: #6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this issue Jul 11, 2016
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

PR-URL: #6432
Fixes: #6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this issue Jul 12, 2016
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

PR-URL: #6432
Fixes: #6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this issue Jul 12, 2016
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

PR-URL: #6432
Fixes: #6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this issue Jul 14, 2016
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

PR-URL: #6432
Fixes: #6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this issue Jul 14, 2016
assert.deepEqual() and assert.deepStrictEqual() will no longer throw a
RangeError if passed objects with circular references.

PR-URL: #6432
Fixes: #6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
josephg added a commit to josephg/node that referenced this issue Mar 31, 2017
Cleaned up as per comments in issue

Ref: nodejs#6416
jasnell pushed a commit that referenced this issue Jun 2, 2017
Fixes: #13314
Refs: #6416

This commit changes semantics of the memos cycles tracker. Before
the change it was used to track all currently wisited nodes of an
object tree, which is a bit shifted from its original intention of
tracking cycles. The change brings intended semantics, by tracking
only objects of the current branch of the object tree.

PR-URL: #13318
Fixes: #13314
Ref: #6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
jasnell pushed a commit that referenced this issue Jun 5, 2017
Fixes: #13314
Refs: #6416

This commit changes semantics of the memos cycles tracker. Before
the change it was used to track all currently wisited nodes of an
object tree, which is a bit shifted from its original intention of
tracking cycles. The change brings intended semantics, by tracking
only objects of the current branch of the object tree.

PR-URL: #13318
Fixes: #13314
Ref: #6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. confirmed-bug Issues with confirmed bugs.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants