Potential semaphore queue overflow of expired tasks

[nikkki]

If getting 'Semaphore timeout' exception in those 2 methods(Client.apiws and Client.api) we don't reach semaphore.leave. Therefore request which was added to the queue, it wouldn't be removed from the queue. And soon the queue will be busy with tasks with an expired timer

  async api() {
    const { semaphore } = this.application.server;
    await semaphore.enter();
    // ....
  /* Lets assume, we getting 'Simaphore timeout' and this error won't be handled by this method
  So we won't be able to reach semaphore.leave()
  */    
    try {
    // ...
      const proc = this.application.runScript(method, sandbox);
      const result = await proc(context)(args);
   // ...
    } catch (err) {
      this.application.logger.error(err.stack);
      this.error(this.res, err.message === 'Not found' ? 404 : 500);
    }
    semaphore.leave();
  }
}

[tshemsedinov]

Thanks @nikkki ! See fixed code.

[nikkki]

@tshemsedinov but we still have the same problem: the queue isn't freed of task after a timeout exception. Add semaphore.leave(); to sort out it.

    try {
      await semaphore.enter();
    } catch {
      semaphore.leave(); // add this line
      this.error(504);
      return;
    }

[tshemsedinov]

@nikkki see this place: lib/semaphore.js#L14 - counter will not be decreased if semaphore queue will reject promise by timeout. So we need no semaphore.leave(); here.

[nikkki]

@tshemsedinov yeah, I agree with you about the counter. But my point was about another thing. I will try to explain my concern in the following example: Let's assume a situation : counter = 0; and this.queue.length < this.size and later timeout exception acts. So in this case, when we calling semaphore.add():

 enter() {
    return new Promise((resolve, reject) => {
      // 1. app skips this if-clause, because counter = 0;
      if (this.counter > 0) {
        this.counter--;
        resolve();
        return;
      }
      // 2. app skips this if-clause, because queue.length < size
      if (this.queue.length >= this.size) {
        reject(new Error('Semaphore queue is full'));
        return;
      }
     // 3. app starts timer
      const timer = setTimeout(() => {
     // 5. later cb rejects a promise, BUT we don't remove expired task from queue. 
     // Eventually queue will be filled of expired tasks and 
     // they will have never been deleted from the queue. And actually it is my concern.
     // So we have to remove it from q somehow :)
        reject(new Error('Semaphore timeout'));
      }, this.timeout);
     // 4. adding a task into queue 
      this.queue.push({ resolve, timer });
    });
  }