[THRIFT-2932] Node.js Thrift connection libraries throw Exceptions into event emitter

• Type: Bug
• Status: Closed
• Priority: Critical
• Resolution: Fixed
• Affects Version/s: 0.9.2
• Fix Version/s: 0.9.3
• Component/s: Node.js - Library
• Assignee: Randy Abernethy
• Reporter: Tom Croucher
• Created: 07/Jan/15 05:51
• Updated: 21/Jul/15 02:21
• Resolved: 14/Feb/15 23:02

Description

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:

    catch (e) {
      if (e instanceof ttransport.InputBufferUnderrunError) {
        transport_with_data.rollbackPosition();
      }
      else {
        throw e;
      }
    }

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.