Ensure "safe" toJSON doesn't break EventEmitters #388

merged 1 commit into from Dec 9, 2012


None yet

3 participants

othiym23 commented Dec 8, 2012

V8 and / or the JS spec are apparently extremely wily about interpreting values as potential indexes for arrays -- even hex strings can be converted internally back to numbers when passed to Object.defineProperty(). This should fix request/request#387.

None of the tests break with this change, but as the bug is sort of a race condition, I couldn't easily come up with a new test case. The breaking test code on request/request#387 no longer crashes when left running for a decent length of time, at least.

@mikeal mikeal merged commit d734337 into request:master Dec 9, 2012
grncdr commented on ea2f975 Jun 29, 2014

@othiym23 any chance you remember why this change was needed? We're tracking down a strange bug (using an older version of request) and it seems like this might be related.


It was causing this incredibly weird heisenbug where serializing the request using util.inspect was causing it to behave like a sparse array. I can't remember exactly how this manifested itself beyond inexplicable crashes in logging code that were fixed by making unambiguous that this property is a name, not a number. Good luck!


i hate this function so much. this whole failed experiment will be gone in the 3.0 branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment