Comments (7)
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 });
from brakes.
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
from brakes.
Ok, workaround :
var rpshield = (options) => request(options);
var brake = new Brakes(rpshield, { timeOut: 1000 });
from brakes.
@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.
from brakes.
@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.
from brakes.
I am going to close this issue. If you have any other questions, please open a new issue.
from brakes.
I do think the implementation should be changed, because current implementation with checking "hasCallback" will be error-prone.
- Make it explicit, sending in via options:
new Brakes({ fn: function(){}, timeout: 10000 })
new Brakes({ promise: Promise.resolve(), timeout: 10000 })
- 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?
from brakes.
Related Issues (20)
- nvmrc is right? HOT 2
- transpile lib? HOT 2
- Brakes modifies original error message
- Support decorator usage like in java HOT 1
- waitThreshold is too high by default HOT 1
- Update Options HOT 2
- Timing the statInterval based on bucketNum & bucketSpan HOT 2
- Do you want TypeScript types in this repo?
- Remove racist terminology
- GlobalStatsStream does not track all instances after update to Node v12.16.0 HOT 5
- Reported request count never goes down? HOT 4
- Slave circuits not working as per the examples HOT 1
- Single-arg fat arrow function breaks brakes HOT 2
- CircuitBrokenError is taking threshold as failure rate
- CPU growth HOT 6
- Adding isFailure function
- Fallbacks executions HOT 3
- Add isSuccess option?
- Runaway memory growth/CPU utilization HOT 1
- 2.6.0 breaks non-string errors HOT 9
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from brakes.