Handling ENOTFOUND and friends
Opened this issue · 24 comments
This is more of a question than a bug report. Maybe it is a feature request.
When the target URL (my term for "the server on the other side") points to a place that simply does not exist (DNS lookup fails, for example), there does not appear to be a clean way to handle it and respond to the client (say, with an HTML page representing the error).
Example:
const server = require('hoxy').createServer({
// Target a perfectly valid website that just doesn't happen to be operational.
reverse : 'http://wegjwgwr.com'
});
// This is how we find out that there's nowhere to send the request to.
server.on('error', function (err) {
if (err.code === 'ENOTFOUND') {
console.error(
'DNS was unable to find an IP for the target of a request.'
);
}
throw err;
});
// This intercept will never run. There is no one on the other end to respond.
server.intercept(
{
phase : 'response'
},
function (req, res) {
console.log('res:', res);
}
);
server.listen(8000, function (err) {
if (err) {
throw err;
}
console.log('Listening on port', server.address().port);
});Given the above example, visiting localhost:8000 will simply hang. Your browser will not receive a response.
If you add:
process.on('unhandledRejection', function (err) {
throw err;
});... then visiting localhost:8000 won't hang but the Node server will crash.
So the question is, how can a proxy implemented with hoxy be user-friendly and respond to the client?
Would it make sense for intercept callbacks to receive an err parameter?
Does your if (err.code === 'ENOTFOUND') code ever run in the above scenario?
Also, what happens if you do:
process.on('unhandledRejection', function (err) {
console.error(err.stack);
});...in the above? unhandledRejection means a promise somewhere isn't being handled. If Hoxy isn't handling a promise, it would probably be a bug.
Yes, listening for err.code === 'ENOTFOUND' is a successful way to detect this case. So it logs that the DNS lookup failed. If I had a way to respond to the client there, I would be a happy camper. There are other similar scenarios, like err.code === 'ECONNREFUSED' when DNS succeeds but a connection is not accepted. However, ENOTFOUND is a particularly easy one to run into. In my real world app, that is especially true because I am actually extracting a target from the path of the request and passing it to request.fullUrl(), so the target is not known ahead of time ... and user input is prone to typos, etc.
The err.stack is pretty lame. The error was created very deep and just passed on up without being amended.
Error: getaddrinfo ENOTFOUND wegjwgwr.com wegjwgwr.com:80
at errnoException (dns.js:26:10)
at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:77:26)As for the unhandled rejection ...
unhandledRejectionmeans a promise somewhere isn't being handled. If Hoxy isn't handling a promise, it would probably be a bug.
... I agree, although I don't know exactly where this is happening. Or what the handling behavior for this kind of failure should even look like.
When using longjohn, the stack trace output from my example becomes:
Error: getaddrinfo ENOTFOUND wegjwgwr.com wegjwgwr.com:80
at errnoException (dns.js:26:10)
at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:77:26)
---------------------------------------------
at Socket.Readable.on (_stream_readable.js:665:33)
at tickOnSocket (_http_client.js:486:10)
at onSocketNT (_http_client.js:502:5)
at nextTickCallbackWith2Args (node.js:455:9)
at process._tickCallback (node.js:369:17)
---------------------------------------------
at ClientRequest.onSocket (_http_client.js:494:11)
at Agent.addRequest (_http_agent.js:143:9)
at new ClientRequest (_http_client.js:139:16)
at Object.exports.request (http.js:31:10)
at new Provi at new ProvisionableRequest (/Users/sholladay/Code/experiment/hoxy-problem/node_modules/hoxy/lib/cycle.js:141:24)
at Cycle.callee$2$0$ (/Users/sholladay/Code/experiment/hoxy-problem/node_modules/hoxy/lib/cycle.js:418:34)
at tryCatch (/Users/sholladay/Code/experiment/hoxy-problem/node_modules/babel-runtime/regenerator/runtime.js:72:40)
at GeneratorFunctionPrototype.invoke [as _invoke] (/Users/sholladay/Code/experiment/hoxy-problem/node_modules/babel-runtime/regenerator/runtime.js:334:22)
at GeneratorFunctionPrototype.prototype.(anonymous function) [as next] (/Users/sholladay/Code/experiment/hoxy-problem/node_modules/babel-runtime/regenerator/runtime.js:105:21)
at onFulfilled (/Users/sholladay/Code/experiment/hoxy-problem/node_modules/co/index.js:65:19)
at /Users/sholladay/Code/experiment/hoxy-problem/node_modules/co/index.js:54:5
at Cycle.co (/Users/sholladay/Code/experiment/hoxy-problem/node_modules/co/index.js:50:10)
at Cycle._sendToServer (/Users/sholladay/Code/experiment/hoxy-problem/node_modules/hoxy/lib/cycle.js:413:30)
at Proxy.callee$3$0$ (/Users/sholladay/Code/experiment/hoxy-problem/node_modules/hoxy/lib/proxy.js:265:28)
at tryCatch (/Users/sholladay/Code/experiment/hoxy-problem/node_modules/babel-runtime/regenerator/runtime.js:72:40)Obviously this info is less than ideal because it is based off of the compiled code. I see that longjohn supports source maps, so I imagine I could create one for hoxy and use it if necessary.
At any rate, that stack points to these two important lines in the ProvisionableRequest constructor:
this._writable = h.request(opts, this._respProm.resolve);
this._writable.on('error', this._respProm.reject);Thanks. I think that narrows it down quite a bit. I made a PR here: #70
Can you check out that branch and verify it fixes it? If I don't hear back in a day or two I'll just merge that PR and publish it to npm as a patch release.
I think what was happening was this line:
this._writable.on('error', this._respProm.reject);...was pumping errors into the promise before it was yielded, which is what sets up the error handling, thus causing the unhandled rejection. So yeah, definitely a bug in Hoxy.
I feel okay removing that line because that writable object is already being checked for errors here.
I am not sure if this is the right approach. On one hand, with that branch, my repro case doesn't crash even if you have a global unhandledRejection handler that throws. So that seems good.
However, the client-side behavior remains the same in that the proxy never responds. And since it doesn't bubble up as an error event anymore, I can't log about it like I used to:
server.on('error', function (err) {
// This is a workable way to check for DNS problems on hoxy's master branch,
// but this will never run on the fix-unhandled-rejection branch.
// That is probably correct / good, except that there is no replacement.
if (err.code === 'ENOTFOUND') {
console.error(
'DNS was unable to find an IP for the target of a request.'
);
}
throw err;
});I need to find some way to respond to the browser reasonably even if the other side of the fence is an abyss.
This is really two separate issues:
Hoxy fails to handle an error properly
This is just a bug in the existing behavior. My previous fix didn't work, and I think I know why. I pushed an update to the PR branch that should allow capturing the error using on('error').
A way to return something to the client if the server can't be reached
Expected behavior may differ between forward and reverse proxy cases. With a forward proxy, probably the browser should behave like it would without a proxy, e.g. whatever happens when you visit http://dfgsdfhjsdgfsdjhfgj.com/
With a reverse proxy, maybe it should return a 502 Bad Gateway.
But there's also a further question of how to send customized responses to the client when these sort of errors happen, above and beyond the default behaviors listed above. Am I capturing your thoughts here?
Yes, precisely. Sounds like we are on the same page.
As for the first problem, using the latest commit, I am not seeing the behavior you describe about being able to capture via .on('error'). I am able to with the master branch.
I added some changes and a unit test to the PR branch that shows the error event being emitted. Can you pull the branch and re-verify? Also look at the test and LMK if it's replicating your scenario.
That unit test looks great. Will re-test on Tuesday or so.
Wouldn't it be a good idea to let the Request Instance that errored emit the errors ENOTFOUND and ECONNREFUSED. Atleast that's something we would need for James.
Just to confirm what nerdbeere noted: when an error occurs, we (James) need to be able to figure out which "request" is affected so we can update the UI representing that request.
This might be a separate issue, because it looks like sholladay is looking for
Application using proxy is notified of errors via the proxy/HTTP protocol
while nerdbeere and I need:
Application managing proxy is notified of errors via Javascript (the "managing" application is part of the same process as the proxy)
So, different issue?
Yeah, two tickets might help. But I think we need the same things. My app is also a replacement for Charles (minus a fancy UI at the moment). We may report the error to our users in different ways, but wouldn't a James user also see their page never finish loading in the browser? Since a response is never delivered, it can cause onload event handlers to never fire, and so on (which has disastrous effects for many sites these days).
I really want to handle request errors in the scope of an intercept and presumably that would give you what you need too. But even just being able to correlate the error to a specific request, as you describe, would at least let me log the URL associated with it, etc. similar to how you want to stop your spinner.
Yep, I think that we're on the same page: even if James was informed of the failed request, the browser would still be continuously loading.
Fair enough, I'll refrain from creating a new issue, unless it's more convenient for greim.
I know exactly where the error is being thrown. Can someone/everyone describe their ideal behavior and I'll be happy to take a shot at some kind of fix/workaround. @sholladay it sounds like your main pain point is the browser hangs, so I'm thinking a completed 502 response would solve that for you?
Yes, that should suffice. I feel like a nice pattern for this would be:
proxy.intercept('response', function (outRequest, outResponse) {
if (outRequest.err) {
// outResponse is pre-populated as a 502, can be overridden as usual
}
});But whatever works!
I just opened #78 which seems to be a duplicate of this. I am OK with sholladay's suggestion for the error handling API. But we can also do it the same way the http module does. There you attach an error event handler for the request object.
req.on('error', (e) => {
});@bibhas2 Note that you can have a callback executed on-error. In fact, I'm using the error event in James
hoxyServer.on('error', (event) => {
console.warn('hoxy error: ', event);
});
I can't seem to find this anywhere in the docs, strangely.
The issue is that, when errors bubble up this way, right now there's no way of telling which request it's for
@mitchhentges, I agree, that error handling model doesn't tell us which request had failed. Please see #78 for details.
Did this issue ever see a resolution?
@BenAHammond no, it remains a problem. This hit me pretty hard and I had to switch to using h2o2 instead of hoxy for projects where I need to guarantee that the client receives a response (namely, reverse proxies in production). They are both great. But hoxy's error handling needs some love.
If this is biting you but you want to stick with it, I suggest pinning hoxy to version 3.1.3 and installing a global unhandledRejection handler that throws to crash the server, combined with a process manager like PM2 to automatically revive the server. If memory serves, this was the only way I could get the socket to close (albeit abruptly) and commit 6291d95 took that ability away because unhandledRejection is no longer triggered. I don't remember all the details, but I think I had trouble with the error event approach. Anyway, that commit was released in 3.2.0.
@greim @sholladay @BenAHammond - this fix should work (at least in my testes).
Now Hoxy return empty response to a browser (no more timeouts) + you can define error callback (connectErrClbk) to handle connection problems / mock response.
Hoxy.createServer({ certAuthority: { key: _key, cert: _cert },
connectErrClbk: function(err, reqData, respToClient)
{
try {
// 502 with debug text on page - example
const util = require('util');
respToClient.writeHead(502, {'Content-Type': 'text/plain'});
respToClient.write("REQUEST DATA:\n");
respToClient.write(util.inspect(reqData) + "\n");
respToClient.write("CONNECTION ERR:\n");
respToClient.write(util.inspect(err) + "\n");
// 302 redirect to - example
// respToClient.writeHead(302, {'Location': 'https://github.com/'});
} catch (ex) {};
}
});
-> + every connection error printed to log (at 'warn' level)
this.emit('log', {
level: 'warn',
message: `connection error: ` + ex.message,
})