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

feat: semaphore added #72

Merged
merged 13 commits into from
Jun 20, 2017
Merged

feat: semaphore added #72

merged 13 commits into from
Jun 20, 2017

Conversation

helio-frota
Copy link
Member

Also using blue-tape to fix a specific test and
Connects to #60

Also using blue-tape to fix a specific test and
Connects to #60
@ghost ghost assigned helio-frota Jun 13, 2017
@ghost ghost added the in progress label Jun 13, 2017
@coveralls
Copy link

coveralls commented Jun 13, 2017

Coverage Status

Coverage increased (+0.01%) to 99.288% when pulling e62a569 on semaphore into fd0fee2 on master.

@coveralls
Copy link

coveralls commented Jun 13, 2017

Coverage Status

Coverage increased (+0.01%) to 99.288% when pulling 0faa766 on semaphore into fd0fee2 on master.

Copy link
Member

@lance lance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some questions about this that I think we need to figure out the answers to, and possibly have tests for.

  • Does it make sense to always use execution isolation with semaphores? What if there's not really a need for it? Why have it be always on? Reading the Netflix documentation here https://github.com/Netflix/Hystrix/wiki/How-it-Works#semaphores it really only makes sense to use semaphore isolation in certain use cases where the latency is very low and throughput requirements very high. Given that, we have no real way of implementing thread pool isolation/rate limiting, so maybe it does make sense to have it always on. I'm not 100% sure, but I think we should discuss it.
  • What happens to the semaphore when a circuit fires, but then times out? Is it released? We need to know that it is.
  • What happens when the semaphore has reached its limit and semaphore.acquire() is called? Does it return a rejected promise? Immediately? Or something else? We should test for this.
  • The docs should be updated.

@helio-frota
Copy link
Member Author

Does it make sense to always use execution isolation with semaphores? What if there's not really a need for it? Why have it be always on? Reading the Netflix documentation here https://github.com/Netflix/Hystrix/wiki/How-it-Works#semaphores it really only makes sense to use semaphore isolation in certain use cases where the latency is very low and throughput requirements very high. Given that, we have no real way of implementing thread pool isolation/rate limiting, so maybe it does make sense to have it always on. I'm not 100% sure, but I think we should discuss it.

Indeed, also I need to research more about it.

What happens to the semaphore when a circuit fires, but then times out? Is it released? We need to know that it is.

Good question, I'm going to investigate it.

What happens when the semaphore has reached its limit and semaphore.acquire() is called? Does it return a rejected promise? Immediately? Or something else?

We should test for this.

Agree.

The docs should be updated.

yup

@helio-frota
Copy link
Member Author

helio-frota commented Jun 16, 2017

What happens to the semaphore when a circuit fires, but then times out? Is it released? We need to know that it is.

seems like It is not releasing

 Circuit Breaker timeout with semaphore
    ✖ should be equal
    ------------------
      operator: equal
      expected: 10
      actual:   9

update: yes it is released semaphore.acquire() and calling release();
https://travis-ci.org/bucharest-gold/opossum/jobs/243761512#L458

@helio-frota
Copy link
Member Author

@lanceball: not release when CB timeout is a blocker right ?
if yes (I think it is) then maybe we need to test another semaphore lib, because to call the release() we need first the the aquire().then(release => release()) ...

Also, I'm going try to change sem.use() to sem.aquire() and try to call the release() in some place around that timeout code

@coveralls
Copy link

coveralls commented Jun 16, 2017

Coverage Status

Coverage increased (+0.01%) to 99.291% when pulling e181d43 on semaphore into fd0fee2 on master.

@coveralls
Copy link

coveralls commented Jun 16, 2017

Coverage Status

Coverage increased (+0.01%) to 99.291% when pulling 66096e2 on semaphore into fd0fee2 on master.

@helio-frota
Copy link
Member Author

I'm doing another test, but I missed to add more released() calls.
I'll push the changes soon.

@helio-frota
Copy link
Member Author

What happens when the semaphore has reached its limit and semaphore.acquire() is called? Does it return a rejected promise? Immediately? Or something else?

We should test for this.

So after change the code , and adding the release() calls, I noticed that the counter is always zeroed after release () and subsequent fire calls are not affected.

I will send the test code because I may be testing the wrong way. So it's good to have a new view on this part

@coveralls
Copy link

coveralls commented Jun 16, 2017

Coverage Status

Coverage increased (+0.02%) to 99.298% when pulling 31b6d64 on semaphore into fd0fee2 on master.

@helio-frota
Copy link
Member Author

Now I'm using this lib to run the code in parallel :
https://www.npmjs.com/package/run-parallel

interesting because the code "runs" without error.

@helio-frota
Copy link
Member Author

@lance when running in parallel we have this :

  Semaphore capacity limit in parallel

    ✔ semaphore count is: 0 and initial capacity is: 1
    ✔ failures: 0
    ✔ fallbacks: 0
    ✔ successes: 2
    ✔ rejects: 0
    ✔ fires: 4
    ✔ semaphore count is: 0 and initial capacity is: 1
    ✔ failures: 0
    ✔ fallbacks: 0
    ✔ successes: 3
    ✔ rejects: 0
    ✔ fires: 4
    ✔ semaphore count is: 1 and initial capacity is: 1
    ✔ failures: 0
    ✔ fallbacks: 0
    ✔ successes: 4
    ✔ rejects: 0
    ✔ fires: 4
    ✔ This is not in parallel -> semaphore count is: 1 and initial capacity is: 1
    ✔ failures: 0
    ✔ fallbacks: 0
    ✔ successes: 4
    ✔ rejects: 0
    ✔ fires: 4

Also new devDependency added run-parallel: 1.1.6
@coveralls
Copy link

coveralls commented Jun 16, 2017

Coverage Status

Coverage increased (+0.02%) to 99.298% when pulling 3d990c3 on semaphore into fd0fee2 on master.

@helio-frota
Copy link
Member Author

Given that, we have no real way of implementing thread pool isolation/rate limiting, so maybe it does make sense to have it always on. I'm not 100% sure, but I think we should discuss it.

true, with the current code we can see some real changes only when we run the code in parallel.
otherwise, looks like a serial code - like running without semaphore. Since it will release() every time btw.

Also removed two of the existing semaphore tests that were not really
automated tests, but rather some debugging information about how the
semaphore rate limiting works.
@coveralls
Copy link

coveralls commented Jun 19, 2017

Coverage Status

Coverage increased (+0.02%) to 99.298% when pulling b3d7b6f on semaphore into fd0fee2 on master.

Replacing 'await-semaphore' since we need the `test()` function
and there does not appear to be a widely used/maintained semaphore
implementation that provides it and doesn't also depend on the
`async/await` keywords at runtime.
@coveralls
Copy link

coveralls commented Jun 20, 2017

Coverage Status

Coverage increased (+0.1%) to 99.375% when pulling 4d0c057 on semaphore into fd0fee2 on master.

Until we can figure out what's going on with node-gyp and 8.1.2.
@coveralls
Copy link

coveralls commented Jun 20, 2017

Coverage Status

Coverage increased (+0.1%) to 99.375% when pulling 4faf0ca on semaphore into fd0fee2 on master.

@coveralls
Copy link

coveralls commented Jun 20, 2017

Coverage Status

Coverage increased (+0.1%) to 99.375% when pulling 80decd2 on semaphore into fd0fee2 on master.

@coveralls
Copy link

coveralls commented Jun 20, 2017

Coverage Status

Coverage increased (+0.1%) to 99.375% when pulling 2c326cc on semaphore into fd0fee2 on master.

@coveralls
Copy link

coveralls commented Jun 20, 2017

Coverage Status

Coverage increased (+0.1%) to 99.375% when pulling 9feb539 on semaphore into fd0fee2 on master.

@coveralls
Copy link

coveralls commented Jun 20, 2017

Coverage Status

Coverage increased (+0.1%) to 99.375% when pulling 5b00bdc on semaphore into fd0fee2 on master.

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

Successfully merging this pull request may close these issues.

3 participants