Optionally retry requests

[pimterry]

Adds a 'retries' option you can pass to send(), which sets the number of times to retry this request (defaulting to 0).

This is part of the fix for balena-io/balena-sdk#237.

[emirotin]

Please do git rebase against the master as I've merged my PR and published the new version just now.

[emirotin]

Nice job! Please wait for Juan to approve as well

[jviotti]

Very nice! Thanks a lot for the tests :)

[pimterry]

Rebased.

Also, whoops, in starting to put this to use against Resin-SDK I've realised it needs to go a little further. There's an extra change here now that lets you set a default retries in the initial getRequest call, so we can inject the option once in the tests, and have it set the default behaviour for all future send calls from that request instance.

[Page-]

This retriesRemaining = undefined is redundant, it will only be set to undefined if it is already nullish, but then retriesRemaining ?= opts.retries will also set it if it's nullish, making it redundant to explicitly change nulls into undefineds

[pimterry]

Yep, totally agree it doesn't directly do anything. It's there really just to explicitly say upfront that this is an optional argument, and afaik this the easiest way to show that in CS. Personally I quite like it, but it might be more of a TS style; happy to change it if that's not useful/how we do things here.

[Page-]

I personally really dislike it as it makes me think there is some special reason it needs to be undefined rather than just nullish, so I'm watching out for that in the code and it adds processing overhead from that. I'd rather just have it in the jsdoc due to that, also for typescript I would write it as retriesRemaining?: number (or retriesRemaining: number | null, I'm not sure which I prefer yet) rather than retriesRemaining: number | null = undefined

[pimterry]

Ok, that makes sense, I'll move it to the JSDoc.

To briefly dive on the TypeScript diversion (with maybe unwarranted enthusiasm): those two options aren't equivalent in TS, so it's not just style.

Firstly, because x?: number has the same type within the function as x: number | undefined, not x: number | null. retriesRemaining: number | null = undefined isn't valid (assuming strictNullChecks)

Secondly, because the ? doesn't just change the type of the argument inside the function, it also makes the argument optional. With x: number | undefined you can explicitly pass undefined, but you can't not pass any argument at all. With x?: number you can pass undefined implicitly, and I think that's probably the idiomatic option.

Playground example

[Page-]

This can be

.catch (-> retriesRemaining > 0), ->
	exports.requestAsync(options, retriesRemaining - 1)

or

p = p.then ...
if retriesRemaining > 0
	p = p.catch ->
		exports.requestAsync(options, retriesRemaining - 1)
return p

to avoid needing to manually rethrow

[pimterry]

If you want to pull it out like that we can just implicitly return a nice ternary, rather than mutating p:

...

if retriesRemaining > 0 then p.catch ->
    exports.requestAsync(options, retriesRemaining - 1)
else p

Thoughts? Otherwise I'd go for your 2nd option myself - the first for me is a little tricky to parse without having read much CS before. I'm still getting to grips with CS style though.

[Page-]

I prefer the mutating style in that I read it more easily along the lines of "in this case I add an error handler, and in all cases I return this promise", rather than "in this case I add an error handler and return the promise, and in the other case I return the promise" - albeit they're equivalent in function and I would be ok with the ternary version.

As for the first one I suggested, in that style it's a bit ugly for sure so I'd go with one of the others, especially as it's not needed in this case at all because the predicate result is always known in advace. However the way to make it a nicer would be to separate out the predicate function and have it named instead, ie:

shouldRetry = -> retriesRemaining > 0
...
.catch shouldRetry, ->
	exports.requestAsync(options, retriesRemaining - 1)

[Page-]

You could use Promise.method to do:

fetchMock.get 'https://example.com/initially-failing', Promise.method ->
	requestsSeen += 1
	if requestsSeen <= 2
		throw new Error('low-level network error')
	else
		return {
			body: result: 'success'
			headers:
				'Content-Type': 'application/json'
		}

[pimterry]

Definitely better, done.

I've done this as tiny patch I've pushed straight to master, referencing this issue. I assume that's the right approach for small tweaks like this when the PRs already merged - let me know there's a different process I should be following.

[Page-]

Generally even for smaller tweaks we'll open up a PR, that way it will still get some eyes on it in case some small mistake was added by accident - after all it can happen to any of us, especially as the small fixes also tend to be the ones that get paid the least attention to. Actually for a lot of our repos it's impossible to push directly to master and changes have to go via a PR and won't let you merge until the tests have passed as well, it'd be good to set that up on even more repos imo

[emirotin]

I personally like it because of the self-documenting nature

…

On Wed, Dec 14, 2016 at 2:31 PM, Tim Perry ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/utils.coffee <#78>: > @@ -292,10 +293,12 @@ timeoutPromise = (ms, promise) -> # utils.requestAsync({ url: 'http://example.com' }).then (response) -> # console.log(response) ### -exports.requestAsync = (options) -> +exports.requestAsync = (options, retriesRemaining = undefined) -> Yep, totally agree it doesn't directly do anything. It's there really just to explicitly say upfront that this is an optional argument, and afaik this the easiest way to show that in CS. Personally I quite like it, but it might be more of a TS style; happy to change it if that's not useful/how we do things here. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#78>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAgGCJokyKws2xTnmSVKfIEin2g13s74ks5rH9OlgaJpZM4LLwqD> .

-- Eugene Mirotin Senior Frontend Engineer site: Resin.io <https://resin.io/>, twitter: @resin_io <https://twitter.com/resin_io>

[emirotin]

Yeah it's ok for tiny fixes, especially related to code style But anything less trivial should go as a new PR

…

On Wed, Dec 14, 2016 at 2:51 PM Tim Perry ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tests/request.spec.coffee <#78>: > @@ -286,3 +253,49 @@ describe 'Request:', -> foo: 'bar' .get('body') m.chai.expect(promise).to.eventually.become(foo: 'bar') + + describe 'given an endpoint that fails the first two times', -> + + beforeEach -> + requestsSeen = 0 + fetchMock.get 'https://example.com/initially-failing', -> Definitely better, done. I've done this as tiny patch I've pushed straight to master, referencing this issue. I assume that's the right approach for small tweaks like this when the PRs already merged - let me know there's a different process I should be following. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#78>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAgGCHBSOLJ7bkWtcAT5yw4qGO7XKJP0ks5rH9hQgaJpZM4LLwqD> .

[pimterry]

Ok, new patch with review markups for this is up: #81