Are registerIds and unsents equal in length?

[ghominejad]

I think there are some serious problems in the codes. 1 - Consider you have 100 registerIds and 50 of them are unsents. so is this code correct?

// registrationId.length is  100 items
for (i = 0; i < registrationId.length; i += 1) {
    // result.results.length is same as unsents length (50 items)
    if (result.results[i].error === 'Unavailable') {
        unsentRegIds.push(registrationId[i]);
     }
}

https://github.com/ToothlessGear/node-gcm/blob/master/lib/sender.js#L132-L136

2 - If we have some unsents then we have loss the callback results of sent items. consider we have only 1 Unavailable and 99 success items. then it retries to send one unsent and after finishes we have only one result in callback results. https://github.com/ToothlessGear/node-gcm/blob/master/lib/sender.js#L154

3- In gcm spec we should respect Retry-After header

[hypesystem]

1 -

Your example is incorrect. Let's try with a more manageable number of entries. If we try to send to three registration ids, and we fail to send to two of them, our response will look something like this:

{
  //...
  results: [
    { message_id: "something" },
    { error: "Unavailable" },
    { error: "Some other error" }
  ]
}

Notice that the length of result.results is the same as that of registrationIds.

Here we have one request, which succeeded, one which was Unavailable (and should be retried) and one which outright failed (for some other reason). The code you show above will run through these, but the if-clause (if (result.results[i].error === 'Unavailable')) will only match the second item. After the code has run, unsentRegIds will contain exactly one element, the registrationId of the device we couldn't send to.

This code works as it should.

Source for claims about the return format: https://developer.android.com/google/gcm/http.html#response

[hypesystem]

2 -

You are absolutely right! We do lose information if only some registration ids are retried. This is clearly an issue that should be addressed as quickly as possible! In fact, this makes send somewhat unsafe to use (while sendNoRetry is still safe).

I have created a separate issue for this, to keep the discussion concentrated: #93.

[hypesystem]

3 -

Can you elaborate a bit on this?

[hypesystem]

Thank you for the great code review :)

[ghominejad]

@hypesystem But it seems result.results is not always the same as that of registrationIds because :

self.sendNoRetry(message, unsentRegIds, lambda);

You pass unsentRegIds and are less than registrationIds

[hypesystem]

@ghominejad Oh dear! It took me a lot of looking through the code to spot that (and get what you were talking about). You are absolutely right!

[ghominejad]

Also great thanks for making this useful library

[hypesystem]

Wait. Okay. Let me try to annotate some code, assuming the example from before, where we retry 1/3:

// We start with registrationIds = [ a, b, c ]; length = 3
Sender.prototype.send = function(message, registrationIds, retries, callback) {
    // ...
    if (registrationIds.length) { // <-- 3, so yes, we go in here.
        this.sendNoRetry(message, registrationIds, function lambda(err, result) {
            // here, result.results.length is 3, because `sendNoRetry` was called
            //                                            with 3 registration ids

            if (attempt < retries) {
                var sleepTime, unsentRegIds = [], i;

                // we don't err, so we ignore this...
                if (err) {
                    // ...
                } 
                // success but check for bad ids
                else {
                    for (i = 0; i < registrationIds.length; i += 1) {
                        if (result.results[i].error === 'Unavailable') {
                            unsentRegIds.push(registrationIds[i]);
                        }
                    }
                }

                if (unsentRegIds.length) { // <-- length is 1, because 1 is being retried.
                    // ...
                    sleepTime = Math.pow (2, attempt) * backoff;
                    if (sleepTime > Constants.MAX_BACKOFF_DELAY) {
                        sleepTime = Constants.MAX_BACKOFF_DELAY;
                    }

                    setTimeout(function () {
                        self.sendNoRetry(message, unsentRegIds, lambda); 
                            //calling the inner with unsentRegIds, which has 1 element
                    }, sleepTime);
                    attempt += 1;
                }
   // ...

This recursively calls sendNoRetry, with this inner lambda -- we assume everything succeeded this time:

        function lambda(err, result) {
            // here, result.results.length is 1, because `sendNoRetry` was called
            //                                                 with 1 unsentRegId
            // BUT! Note how registrationIds is outside of this scope and IS STILL 3

            if (attempt < retries) {
                var sleepTime, unsentRegIds = [], i;

                // we don't err, so we ignore this...
                if (err) {
                    // ...
                } 
                // success but check for bad ids
                else {
                    for (i = 0; i < registrationIds.length; i += 1) {
                        // We are now looking for result.results[1] and result.results[2] which don't exist
                        if (result.results[i].error === 'Unavailable') {
                            unsentRegIds.push(registrationIds[i]);
                        }
                    }
                }

               //That's an exception right there
   // ...

[hypesystem]

@ghominejad it is primarily @ToothlessGear's, I have just taken over a bit of maintenance recently.

[hypesystem]

@ghominejad Could you make a separate issue for your third point? :)

In gcm spec we should respect Retry-After header

[ghominejad]

@hypesystem We should consider a scenario for calling callbacks. When and How should callback fire? result.results[i] has problem and i think it's good idea we change registrationIds with new values before calling lambda.

[hypesystem]

I think we shuold take this discussion in #93.

[hypesystem]

Closing this, because the issues found here have now been split into separate issues (#93, #94).