Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrating opossum with axios #509

Closed
matteodisabatino opened this issue Dec 4, 2020 · 4 comments
Closed

Integrating opossum with axios #509

matteodisabatino opened this issue Dec 4, 2020 · 4 comments

Comments

@matteodisabatino
Copy link

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
Copy link
Member

lance commented Dec 4, 2020

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 rejected Promise`

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
Copy link
Author

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

lance added a commit to lance/opossum that referenced this issue Dec 4, 2020
When a fallback function is called and throws an exception, the circuit
breaker should catch the exception and return a rejected Promise.

See: nodeshift#509

Signed-off-by: Lance Ball <lball@redhat.com>
lholmquist pushed a commit that referenced this issue Dec 7, 2020
When a fallback function is called and throws an exception, the circuit
breaker should catch the exception and return a rejected Promise.

See: #509

Signed-off-by: Lance Ball <lball@redhat.com>
@lholmquist
Copy link
Member

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

@lholmquist
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants