From ca29827eb0317dd64f5f68b3cf588a3f201311cb Mon Sep 17 00:00:00 2001 From: mLuby Date: Tue, 27 Jun 2017 13:30:08 -0700 Subject: [PATCH] Wrap methods to reset thrift client on bad args (#25) * add test that would catch #24 * fix node thrift error --- dist/browser-connector.js | 25 +++++++++++++++++++- dist/node-connector.js | 25 +++++++++++++++++++- src/mapd-con-es6.js | 48 +++++++++++++++++++++++++++++++++++++++ test/integration.spec.js | 19 +++++++++++++++- 4 files changed, 114 insertions(+), 3 deletions(-) diff --git a/dist/browser-connector.js b/dist/browser-connector.js index dd7b18d3..1c33d91d 100644 --- a/dist/browser-connector.js +++ b/dist/browser-connector.js @@ -396,6 +396,7 @@ }); 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); @@ -1391,9 +1392,31 @@ 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/dist/node-connector.js b/dist/node-connector.js index d38f3420..3ef47f5d 100644 --- 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 index 715721ff..8d542557 100644 --- 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 index ca8d9b49..03e9a6cf 100644 --- 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() + }) + }) + } })