Wrap methods to reset thrift client on bad args (#25)

* add test that would catch #24 * fix node thrift error
heavyai · Jun 27, 2017 · ca29827 · ca29827
1 parent f7b6d28
commit ca29827
Show file tree

Hide file tree

Showing 4 changed files with 114 additions and 3 deletions.
diff --git a/dist/browser-connector.js b/dist/browser-connector.js
diff --git a/dist/node-connector.js b/dist/node-connector.js
@@ -25688,6 +25688,7 @@ module.exports =
 	          });
 	          connection.on("error", console.error); // eslint-disable-line no-console
 	          client = thriftWrapper.createClient(MapDThrift, connection);
+	          resetThriftClientOnArgumentErrorForMethods(_this2, client, ["connect", "createFrontendViewAsync", "createLinkAsync", "createTableAsync", "dbName", "deleteFrontendViewAsync", "detectColumnTypesAsync", "disconnect", "getFields", "getFrontendViewAsync", "getFrontendViewsAsync", "getLinkViewAsync", "getResultRowForPixel", "getServerStatusAsync", "getTablesAsync", "host", "importTableAsync", "importTableGeoAsync", "logging", "password", "port", "protocol", "query", "renderVega", "sessionId", "user", "validateQuery"]);
 	        } else {
 	          var thriftTransport = new Thrift.Transport(transportUrls[h]);
 	          var thriftProtocol = new Thrift.Protocol(thriftTransport);
@@ -26683,9 +26684,31 @@ module.exports =
 	  return MapdCon;
 	}();
 
-	// Set a global mapdcon function when mapdcon is brought in via script tag.
+	function resetThriftClientOnArgumentErrorForMethods(connector, client, methodNames) {
+	  methodNames.forEach(function (methodName) {
+	    var oldFunc = connector[methodName];
+	    connector[methodName] = function () {
+	      for (var _len = arguments.length, args = Array(_len), _key = 0; _key < _len; _key++) {
+	        args[_key] = arguments[_key];
+	      }
 
+	      try {
+	        // eslint-disable-line no-restricted-syntax
+	        return oldFunc.apply(connector, args); // TODO should reject rather than throw for Promises.
+	      } catch (e) {
+	        // `this.output` is the Thrift transport instance
+	        client.output.outCount = 0;
+	        client.output.outBuffers = [];
+	        client.output._seqid = null;
+	        // dereference the callback
+	        client._reqs[client._seqid] = null;
+	        throw e; // re-throw the error to Rx
+	      }
+	    };
+	  });
+	}
 
+	// Set a global mapdcon function when mapdcon is brought in via script tag.
 	if (( false ? "undefined" : _typeof(module)) === "object" && module.exports) {
 	  if (!isNodeRuntime()) {
 	    window.MapdCon = MapdCon;

diff --git a/src/mapd-con-es6.js b/src/mapd-con-es6.js
@@ -131,6 +131,35 @@ class MapdCon {
         )
         connection.on("error", console.error) // eslint-disable-line no-console
         client = thriftWrapper.createClient(MapDThrift, connection)
+        resetThriftClientOnArgumentErrorForMethods(this, client, [
+          "connect",
+          "createFrontendViewAsync",
+          "createLinkAsync",
+          "createTableAsync",
+          "dbName",
+          "deleteFrontendViewAsync",
+          "detectColumnTypesAsync",
+          "disconnect",
+          "getFields",
+          "getFrontendViewAsync",
+          "getFrontendViewsAsync",
+          "getLinkViewAsync",
+          "getResultRowForPixel",
+          "getServerStatusAsync",
+          "getTablesAsync",
+          "host",
+          "importTableAsync",
+          "importTableGeoAsync",
+          "logging",
+          "password",
+          "port",
+          "protocol",
+          "query",
+          "renderVega",
+          "sessionId",
+          "user",
+          "validateQuery"
+        ])
       } else {
         const thriftTransport = new Thrift.Transport(transportUrls[h])
         const thriftProtocol = new Thrift.Protocol(thriftTransport)
@@ -1136,6 +1165,25 @@ class MapdCon {
   }
 }
 
+function resetThriftClientOnArgumentErrorForMethods (connector, client, methodNames) {
+  methodNames.forEach(methodName => {
+    const oldFunc = connector[methodName]
+    connector[methodName] = (...args) => {
+      try { // eslint-disable-line no-restricted-syntax
+        return oldFunc.apply(connector, args) // TODO should reject rather than throw for Promises.
+      } catch (e) {
+        // `this.output` is the Thrift transport instance
+        client.output.outCount = 0
+        client.output.outBuffers = []
+        client.output._seqid = null
+        // dereference the callback
+        client._reqs[client._seqid] = null
+        throw e // re-throw the error to Rx
+      }
+    }
+  })
+}
+
 // Set a global mapdcon function when mapdcon is brought in via script tag.
 if (typeof module === "object" && module.exports) {
   if (!isNodeRuntime()) {

diff --git a/test/integration.spec.js b/test/integration.spec.js
@@ -157,7 +157,7 @@ describe(isNodeRuntime ? "node" : "browser", () => {
     connector.connect((connectError, session) => {
       expect(connectError).to.not.be.an("error")
       session.query(sql, options, (error, data) => {
-        expect(connectError).not.be.an("error")
+        expect(error).not.be.an("error")
         expect(Number(data[0].n)).to.equal(6400)
         done()
       })
@@ -192,4 +192,21 @@ describe(isNodeRuntime ? "node" : "browser", () => {
       })
     })
   })
+
+  if (isNodeRuntime) { // bug only applies to node; in browser thriftTransportInstance is undefined.
+    it("on bad arguments: passes error, flushes internal buffer so next RPC doesn't fail, dereferences callback to avoid memory leak", done => {
+      const BAD_ARG = {}
+      const callback = () => { /* noop */ }
+      connector.connect((_, session) => {
+        expect(() => session.getFields(BAD_ARG, callback)).to.throw("writeString called without a string/Buffer argument: [object Object]")
+        const thriftClient = connector._client[0]
+        const thriftTransportInstance = thriftClient.output
+        expect(thriftTransportInstance.outCount).to.equal(0)
+        expect(thriftTransportInstance.outBuffers).to.deep.equal([])
+        expect(thriftTransportInstance._seqid).to.equal(null)
+        expect(thriftClient._reqs[thriftClient._seqid]).to.equal(null)
+        done()
+      })
+    })
+  }
 })