Auto Retry retries when it should not

[chris-rudmin]

On 4xx client errors, it does not make sense to retry because our input is invalid. The server will reject with the same error if we try with the same input.

[hypesystem]

This is completely correct! It wouldn't be a breaking change, either. If you have the time, please make a PR for this :)

I imagine some change inside Sender#send: https://github.com/ToothlessGear/node-gcm/blob/master/lib/sender.js#L95

[eladnava]

@hypesystem The line has changed since then due to other commits =(

A cool trick I learned for the next time you want to link to a specific line in a file: Click the line in the file on GitHub, then press y on your keyboard, and the browser will refresh with a blob URL:

node-gcm/blob/e71405c812892b63b8d673f071874a515409c243/lib/sender.js#L95

It won't change as the master branch gets updated.

[hypesystem]

Cool :) The fix should probably be somewhere in here:

node-gcm/lib/sender.js

Lines 192 to 199 in e71405c

	if (err) {
	return setTimeout(function() {
	self.send(message, originalRecipient, {
	retries: retries - 1,
	backoff: backoff * 2
	}, callback);
	}, backoff);
	}

[eladnava]

@hypesystem Currently, in the lines you highlighted, we retry no matter what. I think we need to return a flag in sendNoRetry called retry that indicates whether we should retry the request or not, since there is no way to override that behavior right now.

What do you think?

In addition, we should not retry when we receive a 401 Unauthorized status code.

[hypesystem]

Are there any other cases than 401 where we would not retry?

The status codes (ie. 401) is returned from sendNoRetry, so we could just match on it, if this is the only case where we should not retry.

[eladnava]

@hypesystem Yes. Any 4xx is a client error and we should not retry.

[hypesystem]

The easy solution would then be to match the status code and not retry if it is a number 399 < x < 500.

The status code is returned as the error from sendNoRetry

[eladnava]

@hypesystem the request module returns the error code in a property,
wouldn't that be a cleaner solution than to perform a regular expression?
And then, return a retry flag to indicate whether to retry or not.

On Sun, Dec 13, 2015, 12:49 Niels Abildgaard [email protected]
wrote:

The easy solution would then be to match the status code and not retry if
it is a number 399 < x < 500

—
Reply to this email directly or view it on GitHub
#145 (comment)
.

[hypesystem]

Matching on some set propery would be a cleaner solution, yes. But I think the responsibility for deciding whether or not we should retry should be in send, not sendNoRetry.

I think the solution with returning something useful (like a retryableError flag) would be an obvious part of #71 (Better error reporting), but not necessarily this issue.

[chris-rudmin]

While we reviewing the retry code, it might be a good idea to implement using the Retry-After header in 5xx error cases instead of configured backoff delay. Spec can be found here: https://developers.google.com/cloud-messaging/http-server-ref

[eladnava]

@chris-rudmin Good idea. There is already an open issue about Retry-After, we should continue discussion on it in that issue. (#94)

[eladnava]

@hypesystem So should we just check if the err object is a number and in the 4xx range in sender.js#L193?

[hypesystem]

@eladnava that is what I would suggest 😄

[eladnava]

Submitted the PR =) @hypesystem

[hypesystem]

Fixed in your PR @eladnava 😄