node-http-proxy Error emitted on proxy server after original request is aborted

trafficstars

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 the aborted event on a request. aborted is a property, I believe the event should be abort. 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:

Node 12.16.3
http-proxy 1.17.0
express 4.17.1

Reproduction path:

Open server (source code below)
Open a request to the server (I used cURL, you could use your browser) and immediately stop the request
- curl http://127.0.0.1:8080/echo
Observe the following error:

proxyRequest start
proxyRequest Error: socket hang up
    at connResetException (internal/errors.js:608:14)
    at Socket.socketCloseListener (_http_client.js:400:25)
    at Socket.emit (events.js:322:22)
    at TCP.<anonymous> (net.js:672:12) {
  code: 'ECONNRESET'
}

Server code:

var httpProxy = require('http-proxy'),
    express = require('express'),
    http = require('http'),
    httpServer,
    app 
        
function sleep(ms) {
  const date = Date.now()
  let currentDate
  while (!currentDate || currentDate - date < ms) currentDate = Date.now()
}       
      
function proxyRequest(request, response) {
  var proxy = httpProxy.createProxyServer({})

  console.log('proxyRequest start')
  
  proxy.on('proxyReq', (proxyReq, req, res, options) => {
    sleep(5000)
  })
  
  proxy.on('error', err => { 
    console.error('proxyRequest', err)
  })
  
  proxy.web(request, response, { target:'127.0.0.1/demo/api/product' })
}   
    
process.title="test"
app = express()
httpServer = http.createServer(app)
  
app.get('/echo', proxyRequest)
    
app.listen('8080')

Jun 09 '20 15:06 briannielson

There was another issue that referenced this change in Node 10 that I think might be responsible, but I'm unsure. https://github.com/nodejs/node/pull/18868

Jun 09 '20 18:06 briannielson

Is there any progress on this issue?

Jan 10 '21 19:01 kilobyte2007

Changing the name of the event worked for me as well!

Jun 01 '21 20:06 ujwal-setlur

Is there any progress on this issue?

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.

Jun 01 '21 20:06 briannielson

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 aborted event used here does actually exist. That's because req in this context is an IncomingMessage object which does expose this event (see here). So the reported "fix" to rename aborted to abort really just prevents that event from firing (since it doesn't exist), thus preventing the proxyReq.abort() call shown below.

    // Ensure we abort proxy if request is aborted
    req.on('aborted', function () {
      proxyReq.abort();
    });

Ultimately, it's this proxyReq.abort() that ends up triggering the error handler with an ECONNRESET error for the proxied outgoing request to the upstream server. There, the following if statement block is skipped because req.socket.destroyed is false I believe because the socket from the client hasn't yet fully destroyed.

        if (req.socket.destroyed && err.code === 'ECONNRESET') {
          server.emit('econnreset', err, req, res, url);
          return proxyReq.abort();
        }

At this point, the http-proxy error callback or event is triggered and the caller ends up getting the ECONNRESET error for the intentional aborted request to the upstream server.

After some quick testing, simply removing the aborted event handler, while it prevents the http-proxy error event from firing, also appears to prevent the socket from closing normally on the upstream server. At least, I don't see the close event fire there.

Another potential fix I was playing with is changing the if statement shown above in the error handler to use req.aborted instead of req.socket.destroyed. This value will be true after the aborted event is fired, so I think it's safe, but not entirely confident.

        if (req.aborted && err.code === 'ECONNRESET') {
          server.emit('econnreset', err, req, res, url);
          return proxyReq.abort();
        }

Jul 15 '21 12:07 gsf4726

@gsf4726, I believe you are correct. I tested your theory, and indeed, the event handler was never firing after renaming the event from aborted to abort. I implemented your change, and it does seem to work.

Jul 19 '21 23:07 ujwal-setlur

@gsf4726, I can confirm that proposed change allows all tests to pass, so I think we can say that this is a better solution.

Jul 20 '21 00:07 ujwal-setlur

After some quick testing, simply removing the aborted event handler, while it prevents the http-proxy error event from firing, also appears to prevent the socket from closing normally on the upstream server. At least, I don't see the close event fire there.

We haven't seen EMFILE errors on our production servers, so it appears that these sockets are eventually getting cleaned up. I was having issues getting tests to run so I am going to leave this issue open until I have time to spend on getting a PR ready.

Your analysis looks correct and I am inclined to agree with you that that is the better solution. If it is an IncomingMessage the effect of what I did was remove the event listener entirely.

Jul 20 '21 18:07 briannielson

@gsf4726, I can confirm that proposed change allows all tests to pass, so I think we can say that this is a better solution.

@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.

Jul 20 '21 18:07 briannielson

@briannielson @ujwal-setlur I actually had some time today to get back to this. I found that the existing test (should proxy the request and handle timeout error) reproduces a similar failure path with the connection to the client being aborted, but from the proxy side by timing out the socket for the request. In this case, req.socket.destroyed is true (though so is req.aborted) and the current code works.

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.

Jul 20 '21 19:07 gsf4726

Hi Just wanted to know the progress in regards to the merging of this PR thanks.