Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gruntfile.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ module.exports = (grunt) ->
stderr: true
failOnError: true
coverage:
command: 'istanbul cover jasmine-node --captureExceptions test && cat ./coverage/lcov.info | ./node_modules/coveralls/bin/coveralls.js && rm -rf ./coverage'
command: 'istanbul cover jasmine-node --forceexit --captureExceptions test && cat ./coverage/lcov.info | ./node_modules/coveralls/bin/coveralls.js && rm -rf ./coverage'
jasmine:
command: 'jasmine-node --verbose --captureExceptions test'
publish:
Expand Down
27 changes: 27 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ This module shares helpers among all [SPHERE.IO](http://sphere.io/) Node.js comp
* [TaskQueue](#taskqueue)
* [Sftp](#sftp)
* [ProjectCredentialsConfig](#projectcredentialsconfig)
* [Repeater](#repeater)
* [ElasticIo](#elasticio)
* [Mixins](#mixins)
* [Qutils](#qutils)
Expand Down Expand Up @@ -55,6 +56,7 @@ Currently following helpers are provided by `SphereUtils`:
- `Sftp`
- `ProjectCredentialsConfig`
- `ElasticIo`
- `Repeater`

#### Logger
Logging is supported by the lightweight JSON logging module called [Bunyan](https://github.com/trentm/node-bunyan).
Expand Down Expand Up @@ -194,6 +196,31 @@ Following files are used to store the credentials and would be searched (descend
#### ElasticIo
_(Coming soon)_

#### Repeater

Repeater is designed to repeat some arbitrary function unless the execution of this function does not throw any errors

Options:

* **attempts** - Int - how many times execution of the function should be repeated until repeater will give up (default 10)
* **timeout** - Long - the delay between attempts
* **timeoutType** - String - The type of the timeout:
* `'constant'` - always the same timeout
* `'variable'` - timeout grows with the attempts count (it also contains random component)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this concept a little better? It's not really clear...


Example:

```coffeescript
repeater = new Repeater {attempts: 10}

repeater.execute
recoverableError: (e) -> e instanceof ErrorStatusCode and e.code is 409
task: ->
console.info("get some stuff..")
console.info("update some another things...")
Q("Done")
```

### Mixins
Currently following mixins are provided by `SphereUtils`:

Expand Down
64 changes: 64 additions & 0 deletions src/coffee/helpers/repeater.coffee
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
Q = require 'q'
_ = require 'underscore'

###*
* Repeater is designed to repeat some arbitrary function unless the execution of this function does not throw any errors
*
* Options:
* attempts - Int - how many times execution of the function should be repeated until repeater will give up (default 10)
* timeout - Long - the delay between attempts
* timeoutType - String - The type of the timeout:
* 'constant' - always the same timeout
* 'variable' - timeout grows with the attempts count (it also contains random component)
###
class Repeater
constructor: (options = {}) ->
@_attempts = if options.attempts? then options.attempts else 10
@_timeout = if options.timeout? then options.timeout else 100
@_timeoutType = options.timeoutType or 'variable'

###*
* Executes arbitrary function
*
* Options:
* task - () => Promise[Any] - the task that should be executed
* recoverableError - Error => Boolean - function that decides, whether an error can be recovered by repeating the task execution
###
execute: (options) ->
throw new Error '`task` function is undefined' unless _.isFunction(options.task)
throw new Error '`recoverableError` function is undefined' unless _.isFunction(options.recoverableError)

d = Q.defer()

@_repeat(@_attempts, options, d, null)

d.promise

_repeat: (remainingAttempts, options, defer, lastError) ->
{task, recoverableError} = options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here as well, and use defaults maybe

{task, recoverableError} = _.defaults options,
  task: # something
  recoverableError: # something

or throw an Error if those values aren't there


if remainingAttempts is 0
defer.reject new Error("Unsuccessful after #{@_attempts} attempts. Cause: #{lastError.stack}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why there are 2 attempts variables here? ...just trying to understand

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if lastError is null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attempts is remaining attempts and @_attempts is how many attempts are allowed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lastError will never be null if attempts is 0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe a better naming here? Like remainingAttempts and @_allowedAttempts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think attempts is ok... in context of this function it should be relatively clear what it does, taking into the consideration that it's private method and the whole class is like 30 LOC :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I still would like to have the other renamed to remainingAttempts though :)
Both with the same name is kind of misleading...

else
task()
.then (res) ->
defer.resolve res
.fail (e) =>
if recoverableError(e)
Q.delay @_calculateDelay(remainingAttempts)
.then (i) =>
@_repeat(remainingAttempts - 1, options, defer, e)
else
defer.reject e
.done()

_calculateDelay: (attemptsLeft) ->
if @_timeoutType is 'constant'
@_timeout
else if @_timeoutType is 'variable'
tried = @_attempts - attemptsLeft - 1
(@_timeout * tried) + _.random(50, @_timeout)
else
throw new Error("Unsupported timeout type: #{@_timeoutType}")

exports.Repeater = Repeater
1 change: 1 addition & 0 deletions src/coffee/main.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ exports.Logger = require './helpers/logger'
exports.TaskQueue = require './helpers/task-queue'
exports.Sftp = require './helpers/sftp'
exports.ProjectCredentialsConfig = require './helpers/project-credentials-config'
exports.Repeater = require('./helpers/repeater').Repeater
exports.ElasticIo = require './helpers/elasticio'

# mixins
Expand Down
70 changes: 70 additions & 0 deletions src/spec/helpers/repeater.spec.coffee
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
Q = require 'q'
_ = require 'underscore'
_.mixin require('underscore.string').exports()

{Repeater} = require '../../lib/helpers/repeater'

describe 'Repeater', ->
it 'should repeat task until it returns some successful result', (done) ->
repeated = 0

new Repeater
attempts: 10
timeout: 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will set timeout to 100 based on this, since 0 is considered false

@_timeout = options.timeout or 100

If you allow to pass 0 maybe you should adjust the null check, or better use defaults

@_options = _.defaults options,
  attempts: 10
  timeout: 100
  timeoutType: 'variable'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:( yeah, you are right...

.execute
recoverableError: (e) -> e.message is 'foo'
task: ->
repeated += 1

if repeated < 3
Q.reject(new Error('foo'))
else
Q("Success")
.then (res) ->
expect(repeated).toEqual 3
expect(res).toEqual "Success"
done()
.fail (error) ->
done(error)

it 'should boubble up unrecoverable errors', (done) ->
repeated = 0

new Repeater
attempts: 10
timeout: 0
.execute
recoverableError: (e) -> e.message is 'foo'
task: ->
repeated += 1

if repeated < 3
Q.reject(new Error('foo'))
else if repeated is 3
Q.reject(new Error('baz'))
else
Q("Success")
.then (res) ->
done("Error was not produced.")
.fail (error) ->
expect(repeated).toEqual 3
expect(error.message).toEqual "baz"
done()

it 'should boubble up an error after provided number of attempts', (done) ->
repeated = 0

new Repeater
attempts: 3
timeout: 0
.execute
recoverableError: (e) -> e.message is 'foo'
task: ->
repeated += 1
Q.reject(new Error('foo'))
.then (res) ->
done("Error was not produced.")
.fail (error) ->
expect(repeated).toEqual 3
expect(_.startsWith(error.message, 'Unsuccessful after 3 attempts')).toBe true
done()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see more test cases:

  • when errors are thrown
  • when you don't pass any options
  • ...