What if something is a callback/async function AND a Promise

[awilkins]

Working with request-promise ;

e.g.

var rp = require('request-promise');
var Brakes = require('brakes');

var options = {
  uri: 'http://some-json-api.com/',
  json: true
};

var brake = new Brakes(request, { timeOut: 1000 });

return brake.exec(options)
  .then( response => {
    return processResponse(response);
  })
  .catch(e => {
    return processError(e.statusCode);
  });

request-promise wraps the normal request object, which is a function that has a callback parameter, and makes it return a promise. Wrapping it causes the behaviour to change because Circuit prefers async callback functions ; instead of the usual behaviour with these options which is for response to be an object representing the processed JSON response, and for errors to end up in .catch, all responses including errors end up in the .then() block because Circuit uses the default promise callback style instead of request-promise's Promise wrapping.

Not sure the best way to resolve this but maybe an option to force the Promise treatment instead of using the callback style? Not a big thing to write code to work around this (and ditch request-promise in the process, since we're not using it's features).

[awilkins]

Ok, workaround :

var rpshield = (options) => request(options);

var brake = new Brakes(rpshield, { timeOut: 1000 });

[awolden]

Hey @awilkins, I have used request-promise in some projects, and I have generally found that it is best to directly use or wrap request. Here is an example of how I wrap it in a promise for use in brakes.

function _requestWrapper(requestOpts) {
  return new Promise((resolve, reject) => {
    request(requestOpts, (err, response) => {
      if (err) {
        return reject(err);
      }
      // check status codes
      const errored = [500, 501, 502, 503, 504, 505].indexOf(response.statusCode) > -1;
      if (errored) {
        return reject(new InvalidResponseError(
          requestOpts.url,
          response.statusCode,
          response.body));
      }
      return resolve(response);
    });
  });
}

const brake = new Brakes(_requestWrapper, { timeOut: 1000 });

[eleonora-ciceri]

@awolden I am following this solution (using the _requestWrapper function proposed above, plus a brake):

function _requestWrapper(requestOpts) {
    return new Promise((resolve, reject) => {
        request(requestOpts, (err, response) => {
            if (err) {
                return reject(err);
            }
            return resolve(response);
        });
    });
}

function returnSomethingFromAService() {
  const brake = new Brakes(_requestWrapper, {timeout: 10000}),
      options = {
          uri: 'my_service_uri'
      };

  return brake.exec(options)
      .then(res => {
          return <something made with my response>;
      })
      .catch(() => {throw new ServiceUnavailable('The microservice is not available');});
}

However, I cannot figure out how to make this work. Suppose that the service I am requesting things to is currently down. I would like to:

• make the request to the service indicated by the URI
• wait just a second (and see that the brake somehow keeps performing requests to such service)
• start the service indicated by the URI
• see that the brake stood in place and waited for the service to start before timeout, returning correctly the response

This would be compliant with a circuit breaker's definition:

with a circuit breaker, after a certain number of requests to the downstream resource have failed, the circuit breaker il blown (S. Newman, Building Microservices)

but actually does not happen: the ServiceUnavailable error is thrown instantly, without waiting for anything. So maybe I am misinterpreting wrongly the documentation (since it is not largely commented). I would expect your brake to try for a while to perform the request, and then, if the request goes well before timeout, to return correctly the response, otherwise (if the timeout passes), to throw a ServiceUnavailable error. How can I achieve this, please?

Thanks.

[awolden]

@Eleanore The problem you are trying to solve is not a problem that circuit breakers were meant to solve. If a request fails instantly, the circuit breaker will not delay the returning of request until the service is ready. It will also not perform retries for you. The purpose of the circuit breaker is to fail fast and take pressure off downstream services when they are faltering. If you want the request to retry you will either have to write the logic yourself or use a lib like node-request-retry

I apologize for any confusion.

[awolden]

I am going to close this issue. If you have any other questions, please open a new issue.

[sveisvei]

I do think the implementation should be changed, because current implementation with checking "hasCallback" will be error-prone.

1) Make it explicit, sending in via options:

new Brakes({ fn: function(){}, timeout: 10000 })

new Brakes({ promise: Promise.resolve(), timeout: 10000 })

2) Make the check inversed, and check if "main/func" is a promise instead of a function with named arguments. e.g. switch here

new Brakes(Promise.resolve(), {timeout: 10000 })

I tested options 2, but the test-suite blew up.

Want a PR for either of these?

[awolden]

Hey @sveisvei,

Thanks for the feedback, but there is a problem with both of your examples and the suggestion to check if the passed function is a promise instead of doing the check to see if it is a callback. In your examples you use Promise.resolve() which returns a pre-resolved promise. However, the concept of brakes requires you to pass in a function reference that returns a promise (i.e. (opts) => Promise.resolve()). This is a subtle but important distinction. The first argument has to be a function reference because you may pass in different options to each request, and you will want a new execution on each call instead of referencing the same resolved promise. Moreover, with the lack of return typing in javascript, it is near impossible to do static analysis to determine if a function returns a promise.

These are all valid promise functions in js:

() => Promise.resolve();
() => new Promise(resolve => resolve());
() => {
  const deferred = Q.deferred;
  deferred.resolve();
  return deferred.promise;
}

You can't realistically determine if a function returns a promise until after you have executed it, whereas with callback functions you can perform analysis on the function signature. If the analysis of the function signature is inadequate, that is something we can and should improve, but I don't see how we can replace it with promise detection.

However, after looking at your examples, I think there is an improvement that could be made. Instead of relying solely on the callback detection in hasCallback we can add an option that forces brakes to treat the first argument as a callback-function.

That would look something like:

const brake = new Brakes((foo, bar) => bar(), {isFunction: true});

In that example, if the isFunction flag is set to true, it forces brakes to treat the first argument as a callback instead of a promise.

Likewise we could add:

const brake = new Brakes((foo, cb) => Promise.resolve(), {isPromise: true});

I would be excited and eager to approve a contribution that makes that change 👍

-Alex Wolden