nodeshift / opossum

Node.js circuit breaker - fails fast ⚡️
https://nodeshift.dev/opossum/
Apache License 2.0
1.31k stars 107 forks source link

Integrating opossum with axios #509

Closed matteodisabatino closed 3 years ago

matteodisabatino commented 3 years ago

Hi, I'm trying to integrate oppossum with axios but I noticed a strage behaviour (in my opinion):

I'm trying to make an HTTP request that must fail with and without opossum.

Without opossum I make the request this way and it works as excepted:

const main = async () => {
  try {
    const response = await axios({
      method: 'POST',
      url: 'http://www.mywebsite.com/'
    })

    console.log('SUCCESS')
    console.log('response: ', response)
  } catch (e) {
    console.log('FAILURE')
    console.log('e: ', e) // code goes here and that's correct
  }
}

main()
  .catch(ex => {
    console.log('Uncaught excpetion: ', ex)
    process.exit(1)
  })

Instead with opossum I make the request this way:

const makeHTTPRequest = args => {
  const breaker = new CircuitBreaker(axios, {
    timeout: 3000,
    errorThresholdPercentage: 50,
    resetTimeout: 30000
  })

  breaker.fallback(fArgs => console.log('FUNCTIONC FALLBACK, fArgs: ', fArgs))

  breaker.on('halfOpen', fArgs => console.log('EVENT halfOpen, fArgs: ', fArgs))
  breaker.on('close', fArgs => console.log('EVENT close, fArgs: ', fArgs))
  breaker.on('open', fArgs => console.log('EVENT open, fArgs: ', fArgs))
  breaker.on('shutdown', fArgs => console.log('EVENT shutdown, fArgs: ', fArgs))
  breaker.on('fire', fArgs => console.log('EVENT fire, fArgs: ', fArgs))
  breaker.on('cacheHit', fArgs => console.log('EVENT cacheHit, fArgs: ', fArgs))
  breaker.on('cacheMiss', fArgs => console.log('EVENT cacheMiss, fArgs: ', fArgs))
  breaker.on('reject', fArgs => console.log('EVENT reject, fArgs: ', fArgs))
  breaker.on('timeout', fArgs => console.log('EVENT timeout, fArgs: ', fArgs))
  breaker.on('success', fArgs => console.log('EVENT success, fArgs: ', fArgs))
  breaker.on('semaphoreLocked', fArgs => console.log('EVENT semaphoreLocked, fArgs: ', fArgs))
  breaker.on('healthCheckFailed', fArgs => console.log('EVENT healthCheckFailed, fArgs: ', fArgs))
  breaker.on('fallback', fArgs => console.log('EVENT fallback, fArgs: ', fArgs))
  breaker.on('failure', fArgs => console.log('EVENT failure, fArgs: ', fArgs))

  return breaker.fire(args)
}

const main = async () => {
  try {
    const response = await makeHTTPRequest({
      method: 'POST',
      url: 'http://www.mywebsite.com/'
    })

    console.log('SUCCESS')
    console.log('response: ', response) // code goes here and that's NOT correct
  } catch (e) {
    console.log('FAILURE')
    console.log('e: ', e)
  }
}

main()
  .catch(ex => {
    console.log('Uncaught excpetion: ', ex)
    process.exit(1)
  })

In particular, in the second case, I receive this output:

EVENT fire, fArgs:  [ { method: 'POST', url: 'http://www.mywebsite.com/' } ]
EVENT failure, fArgs:  The whole axios error
EVENT open, fArgs:  undefined
FUNCTIONC FALLBACK, fArgs:  { method: 'POST', url: 'http://www.mywebsite.com/' }
EVENT fallback, fArgs:  undefined
SUCCESS
response:  undefined

Since the route I'm trying to call doesn't exist, I receive 404 and axios (correctly) throws and exception; so I expected that opossum to have thrown an exception too. Instead it doesn't happen: opossum says the request is completed with success.

Could someone explain me why this strange behaviour?

lance commented 3 years ago

Hi @matteodisabatino - a few things going on here. First of all, you should only be creating a single CircuitBreaker. In your example code, a new one is created with each call to makeHTTPRequest(). If you create a new CircuitBreaker each time you want to invoke your function, that defeats the purpose, as state will never be maintained.

Regarding the fallback function behavior, it's perhaps a little unintuitive (and maybe better docs are needed). If you provide a fallback function there are two possiblities. the fallback succeeds or it fails. In either case, the failure event occurs in order to register the original call as a failure and maintain stats on open/close state, etc.

Scenario 1: If the fallback succeeds, the call to fire() returns a resolved Promise (but remember, the failure event also occurred). Scenario 2: If the fallback fails, the call to fire() returns a rejectedPromise`

Here is your code rewritten to illustrate this.

const axios = require('axios')
const CircuitBreaker = require('./');

const breaker = new CircuitBreaker(axios, {
  timeout: 3000,
  errorThresholdPercentage: 50,
  resetTimeout: 3000
})

breaker.fallback(fArgs => { console.log('FUNCTION FALLBACK, fArgs: ', fArgs); return Promise.reject('Fallback failed')})
breaker.on('halfOpen', fArgs => console.log('EVENT halfOpen, fArgs: ', fArgs))
breaker.on('close', fArgs => console.log('EVENT close, fArgs: ', fArgs))
breaker.on('open', fArgs => console.log('EVENT open, fArgs: ', fArgs))
breaker.on('shutdown', fArgs => console.log('EVENT shutdown, fArgs: ', fArgs))
breaker.on('fire', fArgs => console.log('EVENT fire, fArgs: ', fArgs))
breaker.on('cacheHit', fArgs => console.log('EVENT cacheHit, fArgs: ', fArgs))
breaker.on('cacheMiss', fArgs => console.log('EVENT cacheMiss, fArgs: ', fArgs))
breaker.on('reject', fArgs => console.log('EVENT reject, fArgs: ', fArgs))
breaker.on('timeout', fArgs => console.log('EVENT timeout, fArgs: ', fArgs))
breaker.on('success', fArgs => console.log('EVENT success, fArgs: ', fArgs))
breaker.on('semaphoreLocked', fArgs => console.log('EVENT semaphoreLocked, fArgs: ', fArgs))
breaker.on('healthCheckFailed', fArgs => console.log('EVENT healthCheckFailed, fArgs: ', fArgs))
breaker.on('fallback', fArgs => console.log('EVENT fallback, fArgs: ', fArgs))
breaker.on('failure', fArgs => console.log('EVENT failure, fArgs: ', fArgs))

async function main() {
  return await breaker.fire({
    method: 'POST',
    url: 'http//www.mywebsite.com'
  })
}

main()
  .then(v => console.log('SUCCESS', v))
  .catch(e => console.error('FAILURE', e))

Which produces...

EVENT fire, fArgs:  [ { method: 'POST', url: 'http//www.mywebsite.com' } ]
EVENT failure, fArgs:  [entire axios error]
EVENT open, fArgs:  undefined
FUNCTION FALLBACK, fArgs:  { method: 'POST', url: 'http//www.mywebsite.com' }
EVENT fallback, fArgs:  Promise { <rejected> 'Fallback failed' }
FAILURE Fallback failed

If you change the fallback to return Promise.resolve() you will see that you get the failure event and the circuit opens, but fire() returns a resolved Promise.

EVENT fire, fArgs:  [ { method: 'POST', url: 'http//www.mywebsite.com' } ]
EVENT failure, fArgs:  [entire axios error]
EVENT open, fArgs:  undefined
FUNCTION FALLBACK, fArgs:  { method: 'POST', url: 'http//www.mywebsite.com' }
EVENT fallback, fArgs:  Promise { 'Fallback succeeded' }
SUCCESS Fallback succeeded

One thing I did notice while looking into this is that if the fallback function throws instead of just returning a rejected Promise, it does not behave as expected. Here is the output when an exception is thrown in the fallback function.

❯ node test-throw.js
EVENT fire, fArgs:  [ { method: 'POST', url: 'http//www.mywebsite.com' } ]
EVENT failure, fArgs:  [entire axios error]
EVENT open, fArgs:  undefined
FUNCTION FALLBACK, fArgs:  { method: 'POST', url: 'http//www.mywebsite.com' }
(node:231127) UnhandledPromiseRejectionWarning: Error: Fallback failed
    at Function.<anonymous> (/home/lanceball/src/github.com/nodeshift/opossum/test-throw.js:10:85)
    at fallback (/home/lanceball/src/github.com/nodeshift/opossum/lib/circuit.js:666:10)
    at handleError (/home/lanceball/src/github.com/nodeshift/opossum/lib/circuit.js:657:14)
    at /home/lanceball/src/github.com/nodeshift/opossum/lib/circuit.js:546:17
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
(Use `node --trace-warnings ...` to show where the warning was created)
(node:231127) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 2)
(node:231127) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

This is most definitely a bug. /cc @lholmquist @helio-frota

matteodisabatino commented 3 years ago

@lance thank you very much for your clear explanation :-).

lholmquist commented 3 years ago

Closing this now that #510 has been merged. Will release a new version to npm soonish

lholmquist commented 3 years ago

Release with the bug fix is now released as 5.1.1 on npm