NodeJS crash with TypeError: Cannot read property 'error' of undefined

[rchampeimont]

Version info

• intercom-node version: 2.9.1
• Node version: 6.11.0

Expected behavior

not to crash

Actual behavior

crashes our nodejs application entirely

Steps to reproduce

I don't know how to reproduce the bug but it seems to be incorrect error handling. Perhaps when the API returns some unexpected data? This happened at 2017-11-02 20:16 CET. But it does not happen any more.

Logs

TypeError: Cannot read property 'error' of undefined
1
at callback (/var/app/current/node_modules/intercom-client/dist/client.js line 221 col 29)
var hasErrors = data.error || data.body && data.body.type === 'error.list';
2
at Request._callback (/var/app/current/node_modules/intercom-client/dist/client.js line 137 col 13)
callbackHandler(resolver, r);
3
at self.callback (/var/app/current/node_modules/request/request.js line 186 col 22)
self._callback.apply(self, arguments)
4
at emitOne (events.js line 96 col 13)
5
at Request.emit (events.js line 188 col 7)
6
at Request.onRequestError (/var/app/current/node_modules/request/request.js line 878 col 8)
self.emit('error', error)
7
at emitOne (events.js line 96 col 13)
8
at ClientRequest.emit (events.js line 188 col 7)
9
at TLSSocket.socketErrorListener (_http_client.js line 309 col 9)
10
at emitOne (events.js line 96 col 13)
11
at TLSSocket.emit (events.js line 188 col 7)
12
at emitErrorNT (net.js line 1277 col 8)
13
at (/var/app/current/node_modules/async-listener/glue.js line 188 col 31)
var returned = original.apply(this, arguments);
14
at _combinedTickCallback (internal/process/next_tick.js line 80 col 11)
15
at process._tickDomainCallback (internal/process/next_tick.js line 128 col 9)
16
at process.fallback (/var/app/current/node_modules/async-listener/index.js line 539 col 15)
return fn.apply(this, args || arguments);

[choran]

Hi @almacha
Thanks for this, I will look into into in more detail, like you I cant see at the moment what might be causing this. It is difficult when it is not easily reproducible and only happens intermittently but it should not cause it to crash like you noted.
So I will try and see if I can find any other info but in the meantime let me know if you see it again
Thanks
Cathal

[mlvl42]

The same error happened on the 2017-11-16 at 15:52:13 CET

TypeError: Cannot read property 'error' of undefined
1
at callback (/var/app/current/node_modules/intercom-client/dist/client.js line 221 col 29)
var hasErrors = data.error || data.body && data.body.type === 'error.list';
2
at Request._callback (/var/app/current/node_modules/intercom-client/dist/client.js line 137 col 13)
callbackHandler(resolver, r);
3
at self.callback (/var/app/current/node_modules/request/request.js line 186 col 22)
self._callback.apply(self, arguments)
4
at emitOne (events.js line 96 col 13)
5
at Request.emit (events.js line 188 col 7)
6
at Request.onRequestError (/var/app/current/node_modules/request/request.js line 878 col 8)
self.emit('error', error)
7
at emitOne (events.js line 96 col 13)
8
at ClientRequest.emit (events.js line 188 col 7)
9
at TLSSocket.socketErrorListener (_http_client.js line 309 col 9)
10
at emitOne (events.js line 96 col 13)
11
at TLSSocket.emit (events.js line 188 col 7)
12
at emitErrorNT (net.js line 1277 col 8)
13
at <unknown> (/var/app/current/node_modules/async-listener/glue.js line 188 col 31)
var returned = original.apply(this, arguments);
14
at _combinedTickCallback (internal/process/next_tick.js line 80 col 11)
15
at process._tickDomainCallback (internal/process/next_tick.js line 128 col 9)
16
at process.fallback (/var/app/current/node_modules/async-listener/index.js line 539 col 15)
return fn.apply(this, args || arguments);

[choran]

@mguillau42 thanks for the logs.
I wonder is this to do with the callback where the data is not getting passed through for some reason.

var hasErrors = data.error || data.body && data.body.type === 'error.list';

If data was not returned then I think we would see this error.
I am also checking to see if this is related to #139 or vice versa.
Not sure at the moment so need to dig a bit deeper

[tarunbatra]

+1
Happened with me too, and rather erratically. data is undefined, and causing an uncaughtException.

[choran]

ok, thanks for the update.

[tarunbatra]

Any updates on this?

[choran]

Hi @tarunbatra
No update on this yet. I am still trying to reproduce it here.
If you have any specific data about how to repro it let me know.
As you noted, I think it is related to data being undefined but if you have any other specifics, e.g. when it occurs and when it doesnt that might help.
I will update you when I have more info on this

[igordelorenzi]

Same issue here:
I guess it's a library-related problem. More specifically, the "request" lib.

intercom-node/lib/client.js:

Note that the callback function at line 141 is the same passed at line 138. Somehow, the data part is null or undefined, which is equivalent to response var at the following example (obtained at request lib README):

More details at this link: https://github.com/request/request#custom-http-headers

[eagleeye]

At least I suggest to avoid the crash #182

[choran]

@eagleeye
Really appreciate the PR, will give this a look over and get back to you.
and thanks to everyone here for contributing to the issue.

[jrbaudin]

Just wanna send a +1 on this. Experiencing the same issue. Can't really say what circumstances are causing it. I'm getting it around these lines of ugly code :)

...
intercom.events.listBy({ type: 'user', intercom_user_id: intercomId }, (error, response) => {
  if (!error) {
    _.each(response.body.events, event => {
      eventsList.push(event)
    })
    if (response.body.pages) {
      this.getUserEventsWithPagination(
        intercom,
        intercomId,
        eventsList,
        failedFetches,
        response.body.pages,
        callback
      )
    } else {
      callback(null, 'getUserEventsWithPagination all is good!')
    }
  } else {
    if (_.isEqual(error.statusCode, 429)) {
      console.log('getUserEventsWithPagination ' + error.statusMessage)
    }
    callback(
      {
        code: 'ERROR_CAUGHT',
        message: 'Error on the first try...',
      },
      null
    )
  }
})
...

Specifically..

...
  } else {
    if (_.isEqual(error.statusCode, 429)) {
      console.log('getUserEventsWithPagination ' + error.statusMessage)
    }
    callback(
      {
        code: 'ERROR_CAUGHT',
        message: 'Error on the first try...',
      },
      null
    )
  }
...

Where I'm seeing console.log('getUserEventsWithPagination ' + error.statusMessage) being printed and then the Error arrives

[AceYin0622]

I also have this issue. OS is Redhat and node version 6.12.2, package version 2.9.1

[LewisJEllis]

Also experienced this today, package version 2.9.2 on Node 8.9.4. It was while doing a listBy call to Intercom.

I'm seeing that an error event is emitted from the request object (for some reason, most likely a connection hangup from Intercom's API if I had to guess) and it is not being handled well by intercom-node's usage of the request library.

This line, reproduced here:

this.request(args, (_, r) => {
  callbackHandler(resolver, r);
});

seems to be ignoring the possible error passed to the callback and instead binding it to _ which goes unused. This is a textbook case of how not to do error handling in Node, and it should instead look something like:

this.request(args, (err, r) => {
  if (err) {
    doSomethingElse();
  } else {
    callbackHandler(resolver, r);
  }
});

Ideally the error branch of this would be something like callbackHandler(err), but it seems that the signature of both callback/callbackHandler doesn't leave room to communicate that an error occurred, so the best thing to do here is not immediately obvious. Perhaps some more general thought is needed on robust usage of requests and properly handling/communicating errors.

[kopylash]

We use node-intercom in our Electron app in node process and this error crashes our app almost every time the laptop resumes after sleep. The problem is that there is no internet connection first 20-30 seconds and as @LewisJEllis wrote library does not handle errors from request (ECONRESET, ENOTFOUND, etc). It is easily reproducible if you will try to do any call without network connection.

Again, totally agree with @LewisJEllis that callbackHandler function should be rewritten for a proper error handling and the final result should look like this

 promiseProxy(f, args) {
    if (this.promises || !f) {
      const callbackHandler = this.callback;
      return new Bluebird((resolve, reject) => {
        const resolver = (err, data) => {
          if (err) {
            reject(new Error(JSON.stringify(err)));
          } else {
            resolve(data);
          }
        };
        this.request(args, (err, r) => {
          if (err) {
            reject(err);
          } else {
            callbackHandler(resolver, r);
          }
        });
      });
    } else {
      this.request(args, (err, r) => {
        if (err) {
          // handle error for callbacks
        } else {
          this.callback(f, r)
        }
      });
    }
  }

Hope, it will be fixed soon.

[choran]

I just wanted to note that we are actively looking at this issue.
I think some of the confusion is the mix of callback v promises that we are using.
I think someone noted either here or in another issue that we should not be using callbacks anymore and just use promises going forward.
But for now I think the priority is to fix this issue.
I have been able to repro this but when I tried it in another dev env I was able to handle it.
I think this is again an issue with our promises/callbacks issue but it was confusing the issue a little this week.
So we are reviewing the PRs at the moment should be able to have a fix for this next week.
Thanks and apologies for the delays here.

[maykino]

I'm experiencing the same issue with intercom on shopify. Is there any fix for this already?

[choran]

@maykino Yeah, this should be addressed via #191 which was published over 2 months ago.
This appears (based on feedback and testing) resolve the issue noted here.
Do you think it is a different issue? It might be good to open a separate issue in that case

[choran]

I am going to close this issue since I am unable to repro it and we have been unable to confirm that it is still occurring. Please feel free to open another issue if anyone sees this occurring or a similar issue. It might be easier to track it as a separate issue