Catch JSON.stringfy exceptions instead of silent failing #79
Conversation
|
When this is expected to be merged? |
|
I'm not the maintainer of this project, I've no idea when this will be merged ¯_(ツ)_/¯ |
|
@darrachequesne , when do you expect it can be merged? |
|
@Chopinsky Could you please explain your use case? Why would you try to send data with recursion? |
|
So basically this PR is try to prevent accidentally sending an object with circular reference, which could break the encoding code since |
|
@darrachequesne, can this fix be merged? |
index.js
Outdated
| * so we will invoke callback with an array. | ||
| */ | ||
| try { | ||
| var hasBinary = hasBin(obj.data); |
There was a problem hiding this comment.
Currently, hasBin is only called when the first condition (obj.type === exports.EVENT || obj.type === exports.ACK) is true, but it will always be called here right?
There was a problem hiding this comment.
Good point, I think the try-catch block should be moved into the if statements that checks obj.type. I will update this code tonight.
index.js
Outdated
| try { | ||
| var hasBinary = hasBin(obj.data); | ||
| } catch (e) { | ||
| callback([encodeError()]); |
There was a problem hiding this comment.
I'm wondering whether we should send the encodeError() packet to the other side.
There was a problem hiding this comment.
I think circular reference is something that should be handled at compile time, because a large trunk of data could be discarded because of 1 misplaced object pointer. I can improve the error message to make it more specific on what's the cause of this error, so developers can trace the problem better.
|
How about using the usual |
|
Merged as 92c530d. Thanks! |
This PR is to respond issue #3125 in the main socket.io project: socketio/socket.io#3125.
When JSON.stringfy throws exceptions,
encodeAsStringwould fail silently and hang the channel, as described in the original issue. We will now return the standard error object when an exception is thrown, such that encoding errors can be properly handled.