[
https://issues.apache.org/jira/browse/THRIFT-2932?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14306053#comment-14306053
]
Andrew de Andrade commented on THRIFT-2932:
-------------------------------------------
The general idea is that with NodeJS code you never ever throw (in fact the
only native part of the language that throws on user supplied input AFAIK is
JSON.parse and that's a historical anomaly due to the way Crockford first
implemented it). Instead of throwing you should always return the error back to
it's caller as the first argument of the callback. When that isn't possible,
emitting "error" events is the next best thing. Throwing of an error should
only occur if no callback has been bound to the "error" event. Overall throwing
errors is never used for flow control.
Regarding the specifics for this patch, I have pinged Tom on this issue to get
his feedback. He knows more about what he was thinking when he made these
changes and will be better able to address the points you raised.
I'll dig through these changes in light of your comment and see what else I
might be able to add.
None of the other patches I submitted are dependent on this change, but let me
know if you need me to re-roll them in light of only accepting some of the
changes in this patch.
> Node.js Thrift connection libraries throw Exceptions into event emitter
> -----------------------------------------------------------------------
>
> Key: THRIFT-2932
> URL: https://issues.apache.org/jira/browse/THRIFT-2932
> Project: Thrift
> Issue Type: Bug
> Components: Node.js - Library
> Affects Versions: 0.9.2
> Reporter: Tom Croucher
> Assignee: Randy Abernethy
> Priority: Critical
> Attachments: 0001-minor-node-http-connection-clean-up.patch,
> 0001-nodejs-exceptions-to-callbacks-instead-of-throws-int.patch
>
>
> I've been investigating using the Node.js client in a project however it
> seems like there are instances which don't follow Node.js best practices.
> In particular http_connection.js and connection.js throw errors during
> callbacks. This is considered an anti-pattern in Node because it both removes
> the Exception from the context of the callback making it hard to associate
> with a request as well as throwing it in the context of the EventEmitter code
> which can cause inconsistencies in the Node process.
> This means under some error conditions an uncaught exception would be thrown
> or at least an 'error' event on the singleton client (again removing it from
> the request context).
> Both transport receivers share the same copy-pasta code which contains:
> {code:javascript}
> catch (e) {
> if (e instanceof ttransport.InputBufferUnderrunError) {
> transport_with_data.rollbackPosition();
> }
> else {
> throw e;
> }
> }
> {code}
> I'm working on a patch, but I'm curious about some of the history of the
> code. In particular the exception based loop flow control and the using the
> seqid to track the callback which makes it hard to properly associate it with
> exception handling.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
