From c6f7cea14618d6f22d625c3fc5a39831db4271f7 Mon Sep 17 00:00:00 2001 From: Daisuke Taniwaki Date: Thu, 24 Jun 2021 18:39:33 +0900 Subject: [PATCH 1/4] Handle backend errors --- lib/configproxy.js | 34 +++++++++++++++++++++------------- test/proxy_spec.js | 14 ++++++++++++++ 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/lib/configproxy.js b/lib/configproxy.js index 3b34f429..634fa6ea 100644 --- a/lib/configproxy.js +++ b/lib/configproxy.js @@ -379,19 +379,23 @@ class ConfigurableProxy extends EventEmitter { } targetForReq(req) { - var metricsTimerEnd = this.metrics.findTargetForReqSummary.startTimer(); - // return proxy target for a given url path - var basePath = this.hostRouting ? "/" + parseHost(req) : ""; - var path = basePath + decodeURIComponent(URL.parse(req.url).pathname); - - return this._routes.getTarget(path).then(function (route) { - metricsTimerEnd(); - if (route) { - return { - prefix: route.prefix, - target: route.data.target, - }; - } + return new Promise((resolve, reject) => { + var metricsTimerEnd = this.metrics.findTargetForReqSummary.startTimer(); + // return proxy target for a given url path + var basePath = this.hostRouting ? "/" + parseHost(req) : ""; + var path = basePath + decodeURIComponent(URL.parse(req.url).pathname); + + resolve( + this._routes.getTarget(path).then(function (route) { + metricsTimerEnd(); + if (route) { + return { + prefix: route.prefix, + target: route.data.target, + }; + } + }) + ).catch((e) => reject(e)); }); } @@ -593,6 +597,10 @@ class ConfigurableProxy extends EventEmitter { } }); } + }) + .catch(function (e) { + if (res.finished) throw e; + that.handleProxyError(500, kind, req, res, e); }); } diff --git a/test/proxy_spec.js b/test/proxy_spec.js index 2dfbef66..a659f10c 100644 --- a/test/proxy_spec.js +++ b/test/proxy_spec.js @@ -379,6 +379,20 @@ describe("Proxy Tests", function () { .then(done); }); + it("backend error", function (done) { + var proxyPort = 55550; + util + .setupProxy(proxyPort, { errorTarget: "http://127.0.0.1:55565" }, []) + .then(() => r("http://127.0.0.1:" + proxyPort + "/%")) + .then((body) => done.fail("Expected 500")) + .catch((err) => { + expect(err.statusCode).toEqual(500); + expect(err.response.headers["content-type"]).toEqual("text/plain"); + expect(err.response.body).toEqual("/%"); + }) + .then(done); + }); + it("Redirect location untouched without rewrite options", function (done) { var redirectTo = "http://foo.com:12345/whatever"; util From 68953579699f6b46ab65446ce3274a68cd47f620 Mon Sep 17 00:00:00 2001 From: Daisuke Taniwaki Date: Thu, 24 Jun 2021 18:44:41 +0900 Subject: [PATCH 2/4] Fix lint issues --- lib/configproxy.js | 125 +++++++++++++++++++++++---------------------- 1 file changed, 63 insertions(+), 62 deletions(-) diff --git a/lib/configproxy.js b/lib/configproxy.js index 634fa6ea..768a3b0b 100644 --- a/lib/configproxy.js +++ b/lib/configproxy.js @@ -532,76 +532,77 @@ class ConfigurableProxy extends EventEmitter { var args = Array.prototype.slice.call(arguments, 1); // get the proxy target - return this.targetForReq(req).then((match) => { - if (!match) { - that.handleProxyError(404, kind, req, res); - return; - } + return this.targetForReq(req) + .then((match) => { + if (!match) { + that.handleProxyError(404, kind, req, res); + return; + } - if (kind === "web") { - that.emit("proxyRequest", req, res); - } else { - that.emit("proxyRequestWs", req, res, args[2]); - } - var prefix = match.prefix; - var target = match.target; - that.log.debug("PROXY %s %s to %s", kind.toUpperCase(), _logUrl(req.url), target); - if (!that.includePrefix) { - req.url = req.url.slice(prefix.length); - } + if (kind === "web") { + that.emit("proxyRequest", req, res); + } else { + that.emit("proxyRequestWs", req, res, args[2]); + } + var prefix = match.prefix; + var target = match.target; + that.log.debug("PROXY %s %s to %s", kind.toUpperCase(), _logUrl(req.url), target); + if (!that.includePrefix) { + req.url = req.url.slice(prefix.length); + } - target = URL.parse(target); - if (that.options.clientSsl) { - target.key = that.options.clientSsl.key; - target.cert = that.options.clientSsl.cert; - target.ca = that.options.clientSsl.ca; - } + target = URL.parse(target); + if (that.options.clientSsl) { + target.key = that.options.clientSsl.key; + target.cert = that.options.clientSsl.cert; + target.ca = that.options.clientSsl.ca; + } - // add config argument - args.push({ target: target }); + // add config argument + args.push({ target: target }); - // add error handling - args.push(function (e) { - that.handleProxyError(503, kind, req, res, e); - }); - - // dispatch the actual method, either: - // - proxy.web(req, res, options, errorHandler) - // - proxy.ws(req, socket, head, options, errorHandler) - that.proxy[kind].apply(that.proxy, args); + // add error handling + args.push(function (e) { + that.handleProxyError(503, kind, req, res, e); + }); - // update timestamp on any request/reply data as well (this includes websocket data) - req.on("data", function () { - that.updateLastActivity(prefix); - }); + // dispatch the actual method, either: + // - proxy.web(req, res, options, errorHandler) + // - proxy.ws(req, socket, head, options, errorHandler) + that.proxy[kind].apply(that.proxy, args); - res.on("data", function () { - that.updateLastActivity(prefix); - }); + // update timestamp on any request/reply data as well (this includes websocket data) + req.on("data", function () { + that.updateLastActivity(prefix); + }); - if (kind === "web") { - // update last activity on completion of the request - // only consider 'successful' requests activity - // A flood of invalid requests such as 404s or 403s - // or 503s because the endpoint is down - // shouldn't make it look like the endpoint is 'active' - - // we no longer register activity at the *start* of the request - // because at that point we don't know if the endpoint is even available - res.on("finish", function () { - // (don't count redirects...but should we?) - if (res.statusCode < 300) { - that.updateLastActivity(prefix); - } else { - that.log.debug("Not recording activity for status %s on %s", res.statusCode, prefix); - } + res.on("data", function () { + that.updateLastActivity(prefix); }); - } - }) - .catch(function (e) { - if (res.finished) throw e; - that.handleProxyError(500, kind, req, res, e); - }); + + if (kind === "web") { + // update last activity on completion of the request + // only consider 'successful' requests activity + // A flood of invalid requests such as 404s or 403s + // or 503s because the endpoint is down + // shouldn't make it look like the endpoint is 'active' + + // we no longer register activity at the *start* of the request + // because at that point we don't know if the endpoint is even available + res.on("finish", function () { + // (don't count redirects...but should we?) + if (res.statusCode < 300) { + that.updateLastActivity(prefix); + } else { + that.log.debug("Not recording activity for status %s on %s", res.statusCode, prefix); + } + }); + } + }) + .catch(function (e) { + if (res.finished) throw e; + that.handleProxyError(500, kind, req, res, e); + }); } handleProxyWs(req, socket, head) { From 60c3e60cef9020aabecad02b50235abaeee62839 Mon Sep 17 00:00:00 2001 From: Daisuke Taniwaki Date: Thu, 24 Jun 2021 22:17:25 +0900 Subject: [PATCH 3/4] Remove unnecessary catch --- lib/configproxy.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/configproxy.js b/lib/configproxy.js index 768a3b0b..85f73bf0 100644 --- a/lib/configproxy.js +++ b/lib/configproxy.js @@ -395,7 +395,7 @@ class ConfigurableProxy extends EventEmitter { }; } }) - ).catch((e) => reject(e)); + ); }); } From 61c1278b28d6fdb9dab4f98cbcf3bb72a37b46ac Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 19 Jul 2021 12:47:11 +0200 Subject: [PATCH 4/4] use async def for targetForReq to ensure it returns a promise, no matter where errors may occur --- lib/configproxy.js | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/lib/configproxy.js b/lib/configproxy.js index 85f73bf0..619a8dfc 100644 --- a/lib/configproxy.js +++ b/lib/configproxy.js @@ -378,25 +378,19 @@ class ConfigurableProxy extends EventEmitter { }); } - targetForReq(req) { - return new Promise((resolve, reject) => { - var metricsTimerEnd = this.metrics.findTargetForReqSummary.startTimer(); - // return proxy target for a given url path - var basePath = this.hostRouting ? "/" + parseHost(req) : ""; - var path = basePath + decodeURIComponent(URL.parse(req.url).pathname); - - resolve( - this._routes.getTarget(path).then(function (route) { - metricsTimerEnd(); - if (route) { - return { - prefix: route.prefix, - target: route.data.target, - }; - } - }) - ); - }); + async targetForReq(req) { + var metricsTimerEnd = this.metrics.findTargetForReqSummary.startTimer(); + // return proxy target for a given url path + var basePath = this.hostRouting ? "/" + parseHost(req) : ""; + var path = basePath + decodeURIComponent(URL.parse(req.url).pathname); + var route = await this._routes.getTarget(path); + metricsTimerEnd(); + if (route) { + return { + prefix: route.prefix, + target: route.data.target, + }; + } } updateLastActivity(prefix) {