-
Notifications
You must be signed in to change notification settings - Fork 30
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
.release() does not check actual token count #36
Comments
That's sort of by design to avoid the overhead of the |
The example code was a bit extreme, but it's possible for this to occur accidentally. If the developer was careless with either of the following, then there'll be more tokens than intended.
const sema = new Sema(1);
try {
// code continues even if .acquire() is waiting or failed
sema.acquire().then(x => accessDb(sema)).catch(e => {
console.error(e);
sema.release(); // unnecessary since finally{} will release it
});
} finally {
sema.release();
}
// At this point, there are 2 tokens.
accessDb(sema);
async function accessDb(sema) {
sema.acquire().then(x => {
// do something
sema.release();
});
} I do get what you mean though. There's a lot of ways to use the semaphores incorrectly, we can't catch all of it. But I felt that the semaphore count should at least be consistent, or give warnings if inconsistent. It is a sanity check, if you will. Otherwise it would pretty much go undetected. |
Yeah, that makes sense. I'm not against adding a check if it's seen as a useful addition. |
The
nr
value given to Sema() does not stop.release()
from being called as many times as a user wants.Example below shows an nr value of 1, but the user is able to obtain 2 concurrent uses.
The text was updated successfully, but these errors were encountered: