-
Notifications
You must be signed in to change notification settings - Fork 35
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
What if something is a callback/async function AND a Promise #39
Comments
Ok, workaround : var rpshield = (options) => request(options);
var brake = new Brakes(rpshield, { timeOut: 1000 }); |
Hey @awilkins, I have used 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 }); |
@awolden I am following this solution (using the
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:
This would be compliant with a circuit breaker's definition:
but actually does not happen: the Thanks. |
@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. |
I am going to close this issue. If you have any other questions, please open a new issue. |
I do think the implementation should be changed, because current implementation with checking "hasCallback" will be error-prone.
new Brakes({ fn: function(){}, timeout: 10000 })
new Brakes({ promise: Promise.resolve(), timeout: 10000 })
new Brakes(Promise.resolve(), {timeout: 10000 }) I tested options 2, but the test-suite blew up. Want a PR for either of these? |
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 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 That would look something like: const brake = new Brakes((foo, bar) => bar(), {isFunction: true}); In that example, if the 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 |
Working with request-promise ;
e.g.
request-promise
wraps the normalrequest
object, which is a function that has acallback
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 forresponse
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 defaultpromise
callback style instead ofrequest-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).The text was updated successfully, but these errors were encountered: