.release() does not check actual token count

[cardin]

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();
});

[OlliV]

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.

[cardin]

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.

[OlliV]

Yeah, that makes sense. I'm not against adding a check if it's seen as a useful addition.