ToothlessGear / node-gcm

A NodeJS wrapper library port to send data to Android devices via Google Cloud Messaging
https://github.com/ToothlessGear/node-gcm
Other
1.3k stars 206 forks source link

Request inside lib facing exception #285

Open debjeetsarkar opened 7 years ago

debjeetsarkar commented 7 years ago

This is the stack trace I was able to log when the error occurred.

` at Request._callback (/home/code/node_modules/node-gcm/lib/sender.js:151:24)

at self.callback (/home/code/node_modules/request/request.js:186:22)

at emitOne (events.js:96:13)
at Request.emit (events.js:188:7)
at Request.onRequestError (/home/code/node_modules/request/request.js:845:8)
at emitOne (events.js:101:20)
at ClientRequest.emit (events.js:188:7)
at TLSSocket.socketErrorListener (_http_client.js:310:9)
at emitOne (events.js:96:13)
at TLSSocket.emit (events.js:188:7)
at emitErrorNT (net.js:1278:8)
at _combinedTickCallback (internal/process/next_tick.js:74:11)
at process._tickDomainCallback [as _tickCallback] (internal/process/next_tick.js:122:9)`

I feel this is a concurrency issue where the error callback of the socket is being called.

hypesystem commented 7 years ago

Can you share your exact code you called and some more details about the context?

It is very hard to debug without it.

You could also try running your application with the DEBUG=node-gcm flag, which will print more details.

debjeetsarkar commented 7 years ago

@hypesystem : So, it's pretty straight forward, I always call the send method with a single receiver. Its not breaking always but at times.

var gcmNode = require('node-gcm')
var sender = new gcmNode.Sender('****')
var noOfRetries = 4
var gcm = {}

gcm.send = function(obj, callback) {
    var message = new gcmNode.Message({
        priority: 'high',
        data: {
            messageBody: obj.message,
            notId: parseInt(Math.random() * new Date().getSeconds(), 10),
            payload: obj.payload
        }
    })
    var registrationIds = [];
    registrationIds.push(obj.id)
    sender.send(message, registrationIds, noOfRetries,handleGCMResponse)  
    function handleGCMResponse(err, response)      {
        if (err) {
            callback(err, null)
        } else {
            callback(null, response)
        }
    }
}
hypesystem commented 7 years ago

Hmm it does look like an odd error, and it's very hard to debug if it is so sporadic.

How did you find the original stack trace? Was there an error message? (Just the stack trace, with no error, makes it hard to figure out what exactly went wrong.)

Does the app crash when the error occurs, or does it just return the error?

If it crashes, then it seems like an error in the request lib that isn't returned properly through the callbacks ...

debjeetsarkar commented 7 years ago

@hypesystem , well I came to know about it(accidentally) because I wasn't handling the error correctly on my end and crashed my app server . There was no specific error message as such. I found out the stack trace from the logs. The error comes in the error part of the callback rather than coming in the response part, as we normally get it from the GCM responses.

Let me know your views on this.

hypesystem commented 7 years ago

Ah! Well in that case it seems that the error was passed out correctly, but is indeed "unhandleable". Something went wrong that could have done nothing about.

The correct response in this case is probably to wait for a while and then try again, or to add the notification to an error queue to be handled later. Alternatively, you can simply drop the notification in the rare cases where this happens (if the notification is not important).

Basically: this is indeed an error, and you need to decide what to do about it.

We could probably be better about describing the error. I'll ask again, just to be sure: was there an error message before the stack trace, that explained the reason for the error?

debjeetsarkar commented 7 years ago

@hypesystem , Nope there wasn't . Anyways I am monitoring the error callback now, added some handling and shall communicate here as soon as I face the same error again.

hypesystem commented 7 years ago

Cool! What we could do is wrap all request errors in a nicer error message, so it makes more sense to users.

I'll leave this issue open for that reason :-)