Handle circular references in custom data

[waltjones]

Description of the change

This PR guards against circular references in user data (e.g. custom data object) by cloning the data and removing the circular references.

Why is this necessary?
The linked issue report (#955) notes that the merge function in Rollbar.js errors on the circular references. But JSON serialization will also fail, even if it didn't fail during merge. The circular reference causes an infinite traversal and stack overflow.

Why can't this be solved within the merge function?
That's the first solution I considered. It would have had the advantage of adding circular reference protection everywhere merge is used. However, merge doesn't traverse arrays (except the root, if it is an array), and adding array traversal would change its existing behavior. If there are circular references behind an array, merge will succeed, but JSON serialization will fail.

Why use deepStrictEqual in the node.js tests?
assert.equal, as was previously used for those tests, asserts an identical object reference, not exactly equal objects. Now that the code path clones the user data, the previous assert fails. As a side note, most cases of assert.equal in the node.js tests should really be assert.deepStrictEqual.

Type of change

• Bug fix (non-breaking change that fixes an issue)

Related issues

Fixes #955

Development

• Lint rules pass locally
• The code changed/added as part of this pull request has been covered with tests
• All tests related to the changed code pass in development

Code review

• This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
• "Ready for review" label attached to the PR and reviewers assigned
• Issue from task tracker has a link to this pull request
• Changes have been reviewed by at least one other engineer

[cyrusradfar]

thank you for the context on the bug.

[AndrewJDR]

Hi @waltjones and @cyrusradfar,

Thanks for this, I'm the OP on the #955. I recently updated to this version in a staging environment of a node web service (running nsolid), and I've noticed some lockups at the same time that a rollbar error call is made. nsolid reports long blocking times in rollbar's clone function at this same time:

ID c89dc3a Process blocked for 292 ms at clone (/web/node_modules/rollbar/src/utility.js:404:17) at clone (/web/node_modules/rollbar/src/utility.js:417:28) at clone (/web/node_modules/rollbar/src/utility.js:417:28) at clone (/web/node_modules/rollbar/src/utility.js:417:28) at clone (/web/node_modules/rollbar/src/utility.js:417:28) at clone (/web/node_modules/rollbar/src/utility.js:417:28) at clone (/web/node_modules/rollbar/src/utility.js:417:28) at clone (/web/node_modules/rollbar/src/utility.js:417:28) at clone (/web/node_modules/rollbar/src/utility.js:417:28) at clone (/web/node_modules/rollbar/src/utility.js:417:28)

I don't have perfectly definitive repro yet, but I suspect some type of infinite loop condition. I'm going to try to narrow things down in the coming days, but I wanted to let you know sooner than later. It may not be the best idea to publish this into an npm version yet...

Update: After reverting my staging environment to use an older version of rollbar.js that doesn't have the changes from this PR, I can confirm that the locking up issue hasn't occurred again. I therefore very strongly suspect that this PR's changes is what was causing the lockup. I proposed some ideas of what this could be and how to perhaps fix it here - #991

In lieu of trying some of those potential fixes, I think rollbar.js was probably better off reverting this PR and living with the original issue (#955) than it is now...