request / request

🏊🏾 Simplified HTTP request client.
Apache License 2.0
25.68k stars 3.15k forks source link

Can't catch error using callback and on-error handler #2161

Closed bushev closed 8 years ago

bushev commented 8 years ago

Hello, I can't handle error in the following code:

        this.request.get({
            url: url,
            headers: this.headers
        }, (err, res, body) => {
            if (err) return callback(err);

            callback(err, res, body);
        }).on('error', err => {
            console.log(err.stack);
            callback(err);
        });

I got error output:

Error: read ECONNRESET
    at exports._errnoException (util.js:856:11)
    at TCP.onread (net.js:550:26)

After this my process terminates.

NFNChris commented 8 years ago

I'm having this same issue. I would expect that, if a callback is provided, request would handle any connection related exceptions. Instead I'm getting "Unhandled 'error' event / ECONNREFUSED". I'm writing an application that needs to be robust with respect to handling failed connections. Is this a bug, or am I doing it wrong?

request.post({ url: opts.endpoint, json: change }, function(err) {
  if (err) console.log(err);
});

If this request fails due to connection refused, the exception is not caught, and my app hard stops on an unhandled exception. We never get to the callback.

Running node v4.4.2 and request v2.7.0

keatz55 commented 8 years ago

+1 My app is using request to download/write a large number of images using an async loop. I depend on callbacks to know when all images have been processed, but right now I am only getting 14,895 of 14,970 callbacks, therefore I am assuming that an exception is not being caught in my case as well

simov commented 8 years ago

Can some of you provide a code example to reproduce the bug?

justpaul commented 8 years ago

I'm using request to test a large set of proxies and sometimes I also get this error:

events.js:160
      throw er; // Unhandled 'error' event  
      ^  
Error: read ECONNRESET  
    at exports._errnoException (util.js:1007:11)  
    at TCP.onread (net.js:563:26)  

It happens when I can't connect to proxy and it returns 403 status.

bushev commented 8 years ago

Do not use 'get' or 'post' methods. Use 'request' common method instead. It helps me with the initial problem.

So, my code should be:

        request({
            method: 'GET',
            url: url,
            headers: this.headers
        }, (err, res, body) => {
            if (err) return callback(err);

            callback(err, res, body);
        }).on('error', err => {
            console.log(err.stack);
            callback(err);
        });
josser commented 7 years ago

We have same issue with just the same code:

let reqObj;

try {
 reqObj = request(options, (err, res) => {
   if (err) {
     console.log('got in callback', err);
   }
   console.log(res);
 }).on('error', err => {
   console.log('got in error event', err);
   reqObj.abort();
 });
} catch (e) {
 console.log('Catched', e);
 reqObj.abort();
}

When this code is executed in environments with bad connections it may cause unhadeledException.

 { Error: read ECONNRESET
   at exports._errnoException (util.js:953:11)
   at TCP.onread (net.js:563:26) code: 'ECONNRESET', errno: 'ECONNRESET', syscall: 'read' }

This exception It not catched by catch() or .on('error'), or callback. The only way to handle such error is using process.on('uncacughtException') which is obviously not good practice.

I think issue is somewhere deeper in node.http module in some async function, because the only way to handle it is process.on('uncacughtException') But I don't have enough knowledge to investigate this. The most hard part in investigation is in nature of the issue: it's very hard to simulate such circumstances to reproduce it.
Thank you and sorry for bad english

kazaff commented 7 years ago

@josser i agree with your point, but when use "process.on('uncacughtException')" method, i find that the http connect can not be release, it can make the memory leak and the os connect will be overload.

someone know how to fix the problem?

josephrocca commented 6 years ago

I'm also having this problem. I get this message at events.js line number 163:

Error: read ECONNRESET
  at exports._errnoException (util.js:1050:11)
  at TCP.onread (net.js:582:26)

Exact same uncatchable error as @JustPauL is getting, and I'm also testing a large list of proxies. @josser is right that it's hard to reproduce this. It occurs very rarely and very randomly and it's hard to get any clues at all even if you "pause on uncaught errors" in the inspector. Here's the code (in the most minimal form I could make it) that reproduces this problem (download the zip to get the getNextProxy.js file):

uncatchable-error.zip

const request = require('request');
const getNextProxy = require('./getNextProxy.js'); // this script is in the linked zip file

async function startTester() {

  let proxy, result;
  while( proxy = getNextProxy() ) {

    result = await new Promise(resolve => {
        request({ url:`http://facebook.com`, proxy, timeout:15000 }, function (error, response, body) {
          if(error) resolve(error);
          else resolve(body);
        });
    });

    console.log(String(result).substr(0, 50)+"...");

  }

  console.log("<<TESTER FINISHED>>");

}

// Start testers:
for(let i = 0; i < 30; i++) startTester();

If you run the above script for about 10 minutes (start it with node --inspect-brk .), this eventually happens:

image

It it runs through the full proxy list without the error you'll have to run it again. I'm not sure what's causing it, or how to debug it.

@simov, do you have time to take a quick look at this? Should I post a new issue or can this be re-opened?

Edit 1: I can't even catch this error with process.on('uncaughtException', ...) or process.on('unhandledRejection', ...). I've tested this with node v7.1.0 and v8.1.3 (latest). Continuing investigation and will make further edits if I make any progress.

Edit 2: #1946 seems to be closely related to this issue. I've tried adding .end() to the request so that the request doesn't run via process.nextTick, but this had no effect. Also tried adding a separate on('error', ...) handler, and also wrapped the whole lot in a try/catch. None of that seems to work - still get the same error with the same unhelpful call stack.

Edit 3: May also be related to node/3595 and node/14102 and nodejs/help/705.

Edit 4: I have tried adding

req.on('socket', function(socket) {
  socket.on('error', function (error) {
    reject(error);
    req.abort();
  });
});

as suggested here and here. And have also tried if(response) response.on('error', error => reject(error));, but the error remains uncaught and the script crashes.

mikeal commented 6 years ago

Is this error happening synchronously? Did you try a try/catch?

The expected behavior for errors encountered synchronously in the setup phase is that they would throw.

josephrocca commented 6 years ago

Yep, tried that (mentioned it in my second edit). It's strange though - if it's synchronous, I'd expect the call stack (when the uncaught error is thrown) to contain the original function containing the request.end(), but the bottom of the stack is still _tickCallback.

Edit: Here's the code that I changed for the try/catch + .end() tests:

try {
  request({ url:`http://facebook.com`, proxy, timeout:15000 }, function (error, response, body) {
    if(error) resolve(error);
    else resolve(body);
  }).end();
} catch(e) {
  console.log("CAUGHT", e)
}

And call stack snapshot: image

And to make matters worse I can't even catch the error with these:

process.on('uncaughtException', err => { debugger });
process.on('unhandledRejection', err => { debugger });
josephrocca commented 6 years ago

Here are some people who are/were having a similar problem:

And also, this person says:

I had a similar problem where apps started erroring out after an upgrade of Node. I believe this can be traced back to Node release v0.9.10 this item:

  • net: don't suppress ECONNRESET (Ben Noordhuis)

Previous versions wouldn't error out on interruptions from the client. A break in the connection from the client throws the error ECONNRESET in Node.

But they could handle it because they were directly dealing with the net module. And also the issue there may be slightly different because they seem to be able to handle it with process.on('uncaughtException', ...)


For newbies like me this answer explains node's error event handling well. Makes me think that there's an event handler that hasn't been attached by request, or something like that. As I mentioned above, I tried adding req.on('socket', s => s.on('error', ...)). Any other places that I could try attaching error event handlers?

josephrocca commented 6 years ago

Aaaaand suddenly the error is gone :| I have no idea what I did, but I think it may have had something to do with node versions. I originally had 7.10.0, and tried to upgrade to 8.1.3 with n. It seemed like it upgraded to 8.1.3 fine, but then later I noticed node -v was giving me 7.10.0, so I "properly" upgraded using these instructions and now it seems to be working. To make matters twice as confusing, I tried downgrading to 7.10.0 with n and I can't get the error to happen again! Very confused, but will report back here is anything changes. The most I can deduce from this is that this error is somehow dependent on my nodejs version. Pinging @xl-yang in case this is at all useful to you.

Edit: See my next comment.

xl-yang commented 6 years ago

@josephrocca appreciate for letting me know this. I also tried node 7.10 and node 8.1, the error is still there. Did you checked if there is any change on server side, for example, the server is changed to keep the connection on much longer?

josephrocca commented 6 years ago

Edit: @xl-yang As it turns out, the only thing seems to have changed after playing around with node versions is that the errors are now being caught by process.on('uncaughtException', ...) (this wasn't catching the errors earlier). I'd left this handler in by accident (with just a debugger statement inside) and it was silently swallowing the ECONNRESET errors until eventually it slowed to a halt and I noticed. So basically: nothing has changed. (Though at least I can catch the errors now and fail somewhat gracefully)

Original comment: I'm testing a big list of proxies returned by the npm proxy-list module (see my comment above). It could be that there were a few weird servers causing the problem and that they've changed their behaviour or are no longer returned in the lists, but I did try to narrow down on any particular server that was causing the problem but the error just seemed to occur out of nowhere. I really have no idea at this stage! I'll let you know if I get any more clues.

dominhhai commented 6 years ago

Same problem here :(

JustinLivi commented 6 years ago

Just wondering, why is this issue closed? I have encountered this issue as well, recently.

vipinchacko commented 6 years ago

@keatz55, were you able to solve the issue?

jgimenez commented 6 years ago

Also experiencing this issue, I have a server which proxies stuff between socket.io and tcp connections, so the problem could be in either package.

Same case, nothing of my program in the stack:

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: read ECONNRESET
    at errnoException (net.js:901:11)
    at TCP.onread (net.js:556:19)

and

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: write EPIPE
    at errnoException (net.js:901:11)
    at Object.afterWrite (net.js:718:19)

I have set the an .on("error") handler for both the tcp and the socket.io connections, so I would expect any errors to go there instead of crashing the program. I'm happy to provide more information if someone has ideas.

potatowave commented 6 years ago

I'm also having some struggles around this and would like to know why this is closed?

magicalsociety commented 6 years ago

same

yosiasz commented 6 years ago

any news on this? upgraded to node-v8.9.4-x64. no change

yosiasz commented 6 years ago

Not sure if this will help anyone but once I change my package,json from using nodemon to just plain node all is well. I was using nodemon 1.21.1

  "scripts": {
    "init": "npm install",
    "install": "bower install && tsd install -r -o --save-dev",
    "start": "nodemon src/server/app.js",
    "test": "gulp test"
  },

to

  "scripts": {
    "init": "npm install",
    "install": "bower install && tsd install -r -o --save-dev",
    "start": "node src/server/app.js",
    "test": "gulp test"
  },

upgraded to nodemon 1.15.1 and it works for me

jberall commented 6 years ago

I was having a the same issue when running html-pdf, when I was loading an image from another path.

I changed the

This fixed the problem.

I hope the information is useful.

yousefissa commented 6 years ago

@jberall what did you change? I am still having this issue with my app's outbound requests when behind a proxy.

jberall commented 6 years ago

@yousefissa Instead of loading an image using a path

<img src="http://..."> I converted the image to a base64. so the tag is now <img src="data:image/jpeg;base64,/9j/4AAQSkZJRgABAQEBLAEsAAD//gCUVlQtQ29tcHJlc3MgKHRt..">

I have no idea why the error was triggered but this resolved it.

Good Luck

Dutch-TS-Dev commented 5 years ago

@all, this is one of the most challenging bugs I have faced yet. This page seems to offer a solution as it works for me (however I still need to thoroughly test it, because as others here stated, it might be that I am simply not encountering the error case!) https://github.com/request/request/issues/1946

alexandrucancescu commented 3 years ago

@josephrocca Have you managed to find a solution? I too am testing proxies and have tried multiple http request client, like axios and node-fetch. I get:

Error: Client network socket disconnected before secure TLS connection was established
    at connResetException (internal/errors.js:617:14)
    at TLSSocket.onConnectEnd (_tls_wrap.js:1544:19)
    at TLSSocket.emit (events.js:327:22)
    at endReadableNT (_stream_readable.js:1327:12)
    at processTicksAndRejections (internal/process/task_queues.js:80:21)
Emitted 'error' event on TLSSocket instance at:
    at emitErrorNT (internal/streams/destroy.js:106:8)
    at emitErrorCloseNT (internal/streams/destroy.js:74:3)
    at processTicksAndRejections (internal/process/task_queues.js:80:21) {
  code: 'ECONNRESET',
  path: undefined,
  host: undefined,
  port: undefined,
  localAddress: undefined
}

This throws even though it is wrapped in a try catch, maybe because it originates from processTicksAndRejections. Is that any library that doesn't crash like this when making request to wierd proxy servers?

ChrisMiami commented 3 years ago

I am currently trapping socket.on('error') using HTTPS to serve static and RESTful endpoints. 99% of the crashes I was getting are stopped by socket.on('error') but there's still an ECONNRESET that bypasses my error-handler and spits out a short stack to the console and causes the app to crash. Node v11.6.0.

events.js:173
      throw er; // Unhandled 'error' event
      ^

Error: read ECONNRESET
    at TCP.onStreamRead (internal/stream_base_commons.js:162:27). // return stream.destroy(errnoException(nread, 'read'));
Emitted 'error' event at:
    at emitErrorNT (internal/streams/destroy.js:82:8)
    at emitErrorAndCloseNT (internal/streams/destroy.js:50:3)
    at process.internalTickCallback (internal/process/next_tick.js:72:19)

The code:

(internal/stream_base_commons.js:162:27)  return stream.destroy(errnoException(nread, 'read'));
(internal/streams/destroy.js:82:8)  self.emit('error', err);
(internal/streams/destroy.js:50:3)  emitErrorNT(self, err);
(internal/process/next_tick.js:72:19)  Reflect.apply(callback, undefined, tock.args);

The first few lines of streams/destroy.js look like this:

function destroy(err, cb) {
  const readableDestroyed = this._readableState &&
    this._readableState.destroyed;
  const writableDestroyed = this._writableState &&
    this._writableState.destroyed;

  if (readableDestroyed || writableDestroyed) {
    if (cb) {
      cb(err);
    } else if (err &&
               (!this._writableState || !this._writableState.errorEmitted)) {
      process.nextTick(emitErrorNT, this, err);
    }
    return this;
  }

I'm willing to bet that line 12 is where my woes begin.

So, since domains are pretty-much deprecated, on what should I register an error handler for this? As I said, I've already coded socket.on('error') so it's apparently not happening in the call stack of the socket. I haven't yet found a TCP object and I'm not sure if I can just listen for any and all stream errors or if there's a hidden socket.on('stream')? (spoiler: there's not)

alexandrucancescu commented 3 years ago

I managed to mitigate all this errors using another client node-libcurl, which performs perfectly working with proxies and supports them without any other libraries, if that is the cause of your problem.

If you want to keep using request and get this weird errors because of unstable proxy servers, you should also check replacing proxy-agent ( or whatever you are using for http agent ) with tunnel, only supports http/https proxies, but for me it mitigated my error.

Dutch-TS-Dev commented 3 years ago

Hi Chris, are you still experiencing these errors? It's been a while since I had these issues. Let me know, perhaps we could have a look together

ChrisMiami commented 3 years ago

@alexandrucancescu @WAINGOR Thank you - my app is a server that uses Node's built-in HTTPS and Express and the ECONNRESET is caused by clients, some of which may possibly be automated. This is a current issue for me, and I'm going to add longjohn and trap unhandledexception in order to try to pinpoint how this stream read error is able to sneak past socket.on('error').

alexandrucancescu commented 3 years ago

@ChrisMiami If this can also be triggered by clients in a server, then we should report this problem to the nodejs github, as it sounds like a problem with the underlying net implementation, or the event loop. This could get exploited.

ChrisMiami commented 3 years ago

@DutchJSDev, @alexandrucancescu: I have to retract my post. It turns out there was a hidden listener in one of the libraries I'm using and it was this separate socket server that was dying. I've added socket.on('error') to that listener and now the problem is fixed. My assumption was invalid - socket.on('error') does trap all the errors I'm getting, there is nothing escaping.