Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How can I filter socket hang up errors? #813

Open
JSteunou opened this issue Apr 27, 2015 · 23 comments
Open

How can I filter socket hang up errors? #813

JSteunou opened this issue Apr 27, 2015 · 23 comments

Comments

@JSteunou
Copy link

I'm using node-http-proxy for a while now, and I think I'm using it quite right catching errors like this

proxy.on('error', function(err, req, res) {
    // log
    // ...
    // handle
    if (!res.headersSent) {
        res.writeHead(500, { 'content-type': 'application/json' });
    }
    res.end(JSON.stringify({ error: 'proxy_error', reason: err.message }));
});

But I can have two cases that make a "socket hang up" error happen.

  1. Crash behind
  2. Client cancelling the request

Watching at my log I can make no differences between those two cases and it's quite frustrating. Before knowing about request abort (thank #527) I really though I have an error going on.

So, what's the best practice to filter the request.abort case out?

@theoephraim
Copy link

+1

@JSteunou JSteunou changed the title How can I filter socket hand up errors? How can I filter socket hang up errors? Apr 28, 2015
@FoghostCn
Copy link

+1

@vvo
Copy link

vvo commented Dec 1, 2015

You should be able to use the proxy error event or the error callback and check for res.socket.destroyed === true and error.code === ECONNRESET which means the request was canceled

@JSteunou
Copy link
Author

JSteunou commented Dec 2, 2015

Merci @vvo ;) Will try that asap

@JSteunou
Copy link
Author

JSteunou commented Dec 2, 2015

It works very fine, thank you again @vvo

@jcrugzz should I let this issue open? Maybe http-proxy should filter those error inside the library, or maybe it's better to let it unopinionated...

@vvo
Copy link

vvo commented Dec 2, 2015

or maybe it's better to let it unopinionated...

Yes it depends on the use case, sometime you will want the error to pop out if the client canceled the request.

@JSteunou
Copy link
Author

JSteunou commented Dec 2, 2015

What I thought. Closing it then.

@JSteunou JSteunou closed this as completed Dec 2, 2015
@JSteunou
Copy link
Author

JSteunou commented Dec 2, 2015

Quick question again: I want to use request abort to prevent the underlying server to handle that client cancelled request. But, very surprisingly, req.abort is undefined O_O

Any thoughts?

@vvo
Copy link

vvo commented Dec 2, 2015

nope sorry,

@JSteunou
Copy link
Author

JSteunou commented Dec 2, 2015

I think the req I got is the received request, not the request currently sent by http-proxy

@JSteunou
Copy link
Author

JSteunou commented Dec 2, 2015

Ok for the record, in case anyone was interested at doing the same thing

How to catch request client cancelled and abort ongoing proxied request

proxy.on('proxyReq', function(proxyReq, req) {
    // keep a ref
    req._proxyReq = proxyReq;
});

proxy.on('error', function(err, req, res) {
  if (req.socket.destroyed && err.code === 'ECONNRESET') {
    req._proxyReq.abort();
  }
});

@jcrugzz
Copy link
Contributor

jcrugzz commented Dec 2, 2015

This is a bug and should be fixed within http-proxy itself. Currently we are emitting an error on the client request when we should only be cancelling the proxy request when this happens.

@jcrugzz jcrugzz reopened this Dec 2, 2015
@JSteunou
Copy link
Author

JSteunou commented Dec 2, 2015

Which one? Getting all errors even from client cancelled request or be forced to tricky save proxyReq to be able to abort?

@jcrugzz
Copy link
Contributor

jcrugzz commented Dec 2, 2015

@JSteunou To put it simply, your workaround here should be handled within http-proxy for the req.on('error')

@JSteunou
Copy link
Author

JSteunou commented Dec 2, 2015

Would be nice :)

@jcrugzz
Copy link
Contributor

jcrugzz commented Dec 2, 2015

@JSteunou Pull requests always welcome if you want to take a stab at it! :)

@jcrugzz
Copy link
Contributor

jcrugzz commented Dec 2, 2015

also @vvo if you want the error handled, you could handle the error directly on req.on('error'). Im of the opinion that we need to cleanup our proxyReq regardless and if you want it as an error in your code the error can be handled there. Does this seem reasonable?

@vvo
Copy link

vvo commented Dec 2, 2015

Yep makes sense

@JSteunou
Copy link
Author

JSteunou commented Dec 2, 2015

@jcrugzz I'm afraid I have the necessary solution and motivation but not the time to do it.

@jcrugzz
Copy link
Contributor

jcrugzz commented Dec 2, 2015

@JSteunou it would be a simple change right around here. You may be overestimating the time :). It would be a good opportunity for first contribution

@tkers
Copy link

tkers commented Feb 8, 2016

Fairly old thread, but still an issue afaik. Resolving it by comparing the error code to ECONNRESET and ignoring the error as suggested before.

Would be happy to submit a PR to handle this exception internally, but aren't there any use cases where you'd actually want to handle this manually? At the very least you'd need to emit a new event instead (abort perhaps?). Any thoughts?

Deividy added a commit to Deividy/node-http-proxy that referenced this issue Feb 24, 2016
ECONNRESET means the other side of the TCP conversation abruptly
closed its end of the connection.

If we get an ECONNRESET error from the proxy request and the
socket is destroyed this means that the client has closed
his connection, and emit this as an error can lead to
confusion and hacks to filter that kind of message.

I think that the best we can do is abort and emit another event,
so if any caller wants to handle with that kind of error, he can
by listen the disconnected event.

http-party#813
@cycold
Copy link

cycold commented Jun 3, 2016

mark

jcrugzz pushed a commit that referenced this issue Jun 3, 2016
* Emit disconnected event instead of error when ECONNRESET

ECONNRESET means the other side of the TCP conversation abruptly
closed its end of the connection.

If we get an ECONNRESET error from the proxy request and the
socket is destroyed this means that the client has closed
his connection, and emit this as an error can lead to
confusion and hacks to filter that kind of message.

I think that the best we can do is abort and emit another event,
so if any caller wants to handle with that kind of error, he can
by listen the disconnected event.

#813

* code standards, move econnreset check, change ev name
@flevour
Copy link

flevour commented Oct 16, 2018

Should we consider this closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants