Semaphores bug???

[hhgyu]

work : 100 initialCount: 5

wait called : 100 _count = -95

signal called : 5 _count = -90

Not all of the remaining work is started on standby

function Semaphore(initialCount) {
    this._count = initialCount || 1;
    this._waiting = [];
}

Semaphore.prototype.signal = function () {
    this._count += 1;

    if (this._count > 0) {
        var waiter = this._waiting.shift();
        if (waiter) {
            waiter();
        }
    }
};

Semaphore.prototype.wait = function (cb) {
    this._count -= 1;

    if (this._count < 0) {
        this._waiting.push(cb);
    } else {
        cb();
    }
};

// Semaphores

function Semaphore(initialCount) {
    this._maxCount = initialCount || 1;
    this._count = 0;
    this._waiting = [];
}

Semaphore.prototype.signal = function () {
    this._count-=1;
    if(this._count < this._maxCount) {
        var waiter = this._waiting.shift();
        if (waiter) {
            this._count+=1;
            waiter();
        }
    }
};

Semaphore.prototype.wait = function (cb) {
    if (this._count >= this._maxCount) {
        this._waiting.push(cb);
    } else {
        this._count+=1;
        cb();
    }
};

[ronkorving]

Hi, can I see the code you ran to get that output? Also, are you suggesting that the 2nd block you pasted fixes the issue?

[chriswiggins]

I'm hitting the same problem. It seems to stop after running the initial count?

[ronkorving]

I'm looking into it, but more eyes to try and figure out why it's not working are of course welcome :) I just added a test suite, so we can test the heck out of Semaphore and find the problem perhaps more easily (and prevent regression in the future).

[ronkorving]

@chriswiggins I have a hard time making it break. Can you give me some sample code that breaks?

This code works just fine:

    var sem = locks.createSemaphore(2);  // 2 available resources

    function fn() {
        setTimeout(function () {
            sem.signal();
        }, 0);
    }

    for (var i = 0; i < 100; i++) {
        sem.wait(fn);
    }

    setTimeout(function () {
        assert(sem._waiting.length === 0); // nobody waiting
        assert(sem._count === 2);  // 2 available resources at the very end
    }, 1000);

What am I missing...?

[Tatsujinichi]

var locks = require('locks');

var sem = locks.createSemaphore(5);

function createfunc(i) {
    return function() { console.log(i); };
}

for (i = 0; i < 100; i++)
{
    sem.wait(createfunc(i));
}

for(j = 0; j < 100; j++)
{
    sem.signal();
}

I don't believe there is a bug in the code here. But conceptually. I can't think of a many situations to use a semaphore outside of some sort of producer consumer relationship. Because this doesn't really serve to throttle the producer, things just queue without limit, it doesn't really serve that purpose. "wait" is not really wait. "wait" is enqueue. The plan is to defer execution.

If you neglect to signal within the call back, it won't really simulate a real threaded semaphore, as in my example above. So you might read this code and expect it to print 0 to 99, but in you will you only get 0-9.

If you read the output here from the bug entry: work : 100 initialCount: 5

wait called : 100 _count = -95

signal called : 5 _count = -90

This is unintuitive, but makes sense. If you have a initial count of 5, and call wait 100 times, your first 5 call backs will execute because there are 5 available resources, and then, you will have to signal 95 more times before any more are allowed to execute. It is confusing because when using semaphores normally, your thread will execute and call signal when it is finished. But in this implementation the signal must be called within the callback function to simulate that situation.

[ronkorving]

I think I know what the problem is. I've been working on master, and you've been working on the 0.1.0 release? A fix has been merged since 0.1.0 (merged on March 16) but was never released. I will do a release right now to make sure things are good. Please let me know how things work out for you.

Update: I just released 0.2.0 on NPM.

[ronkorving]

I've now released 0.2.2 on NPM, with 100% unit test coverage. Please let me know if your issues are resolved.

[chriswiggins]

Initial tests are looking good for me!

Thanks for the release.

Chris

On 16/11/2015, at 10:57 PM, Ron Korving notifications@github.com<mailto:notifications@github.com> wrote:

I've now released 0.2.2 on NPM, with 100% unit test coverage. Please let me know if your issues are resolved.

Reply to this email directly or view it on GitHubhttps://github.com/Wizcorp/locks/issues/3#issuecomment-156975276.

[ronkorving]

Thanks for the feedback!

[ronkorving]

I will close this issue now. If any other problems show up, please let me know.