-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Error emitted on proxy server after original request is aborted #1455
Comments
There was another issue that referenced this change in Node 10 that I think might be responsible, but I'm unsure. nodejs/node#18868 |
Is there any progress on this issue? |
Changing the name of the event worked for me as well! |
I did not fix tests, so the PR I posted is not mergable. However, the change appeared to fix the issue in our production environment. |
I ran into this bug as well and looked into it a bit more. I wanted to leave here what I found in case it helps someone (or me in the future if I can get back to it). On a second look, the
Ultimately, it's this
At this point, the http-proxy error callback or event is triggered and the caller ends up getting the After some quick testing, simply removing the Another potential fix I was playing with is changing the if statement shown above in the error handler to use
|
@gsf4726, I believe you are correct. I tested your theory, and indeed, the event handler was never firing after renaming the event from |
@gsf4726, I can confirm that proposed change allows all tests to pass, so I think we can say that this is a better solution. |
We haven't seen Your analysis looks correct and I am inclined to agree with you that that is the better solution. If it is an |
@ujwal-setlur if you end up opening a PR, make sure to reference this issue so it can be closed and so I can get a notification and patch our system. |
@briannielson @ujwal-setlur I actually had some time today to get back to this. I found that the existing test ( I've been working on adding a new test that better represents the real-life case we've run up against where the client aborts the request. I should be able to get a PR together soon. |
Hi Just wanted to know the progress in regards to the merging of this PR thanks. |
+1 I've tested the fix locally and it works great. Would really love to see this merged and released so I can stop monkey-patching |
Further confirming this fix solves my issue which PR #1542 should implement. Thanks guys, that made my life much easier! |
any plans on merging this fix? |
I found that for Node 10+ if a request is cancelled before being proxied, the proxyServer emits an error instead of an
econnreset
event. This does not happen for < Node 8. I believe this is due to a change in the lifetime of error event listeners on the original request socket.I think the underlying issue is that in
lib/http-proxy/passes/web-incoming.js
there is an event listener on theaborted
event on a request.aborted
is a property, I believe the event should beabort
. Making this change fixes the issue in my reproduction path.https://github.com/http-party/node-http-proxy/blob/master/lib/http-proxy/passes/web-incoming.js#L146
Versions:
Reproduction path:
curl http://127.0.0.1:8080/echo
Server code:
The text was updated successfully, but these errors were encountered: