Added semaphore and blue-test for specific failed test. #71
Conversation
There was a problem hiding this comment.
This is a pretty big diff, and it's hard to tell, but it seems the real change is simply wrapping all of the code from fire() within a semaphore.use() scope. Is that the case? Or are there other subtle differences I am missing here?
Also, with regard to the code wrapping, I think it can be narrower in scope. Take a look at my comments below.
Other than these issues, it looks good. Thanks!
| */ | ||
| this.emit('cacheMiss'); | ||
| } | ||
| const semaphore = new Semaphore(this.options.capacity); |
There was a problem hiding this comment.
This should be a class property. Creating a new semaphore each time the circuit fires doesn't achieve the goal of rate limiting, because the semaphore is only ever used once, and cannot block other code paths from acquiring it.
|
|
||
| let timeout; | ||
| let timeoutError = false; | ||
| return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
I think we don't need to call semaphore.use() until here. All of the other stuff should be allowed to execute. E.g. I think it's safe to check for a cache hit and also to check if the circuit is open before attempting to acquire the semaphore. No reason for that stuff to be rate limited.
There was a problem hiding this comment.
Ok! Will change this. Thanks !
|
Yes. |
|
Changes and new PR here #72 |
No description provided.