async-sema
async-sema copied to clipboard
.release() does not check actual token count
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.
const Sema = require("async-sema").Sema;
let i = 0;
const a = new Sema(1);
a.release();
a.acquire().then(() => {
i++;
console.log(`i = ${i}`);
return a.acquire();
}).then(() => {
i++;
console.log(`i = ${i}`);
return a.acquire();
});
That's sort of by design to avoid the overhead of the if
as technically you just shouldn't write such code. Remember that the original intention was to receive and return tokens or instances.
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.
- Pairing: Not one-to-one mapping of .acquire() to .release(), especially due to nested functions or error handling
- Asynchronity: .acquire() occuring after .release() if async operations were not handled correctly
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.