samchon / tstl

TypeScript-STL (Standard Template Library, migrated from C++)
MIT License
602 stars 49 forks source link

it seems that Semaphore try_acquire_for has some bug #70

Closed chacent closed 4 years ago

chacent commented 4 years ago

it occors multiple times ,at last the bug always(at least two diff scene) found at try_acquire_for and release,,but i can't repeat it with simply code. i believe that it has a bug

samchon commented 4 years ago

I'm going to review implementation of the Semaphore class in this weekend, may Sunday. However, it would be better you to provide me more detailed information or some of the test code who can reproduce the bug.

chacent commented 4 years ago

the counter seems going bad when lots of try_acquire_for,at last semaphore can't be acquired. i try to make the scene but i failed,It's not easy to reproduce

chacent commented 4 years ago

i'm sure that my code has release every time,and the function acquire do well always

chacent commented 4 years ago

my code is like this

(async()=>{
    while(true){
        if(await s.try_acquire_for(1000)){
            (async()=>{
                const wait = Math.ceil(Math.random()*10);
                console.log(1,wait);
                await new Promise(s=>setTimeout(s,wait));
            })().finally(()=>s.release());
        }
    }
})();
chacent commented 4 years ago

This is the code that i actually used

thread_middleware() {
    return async(ctx,next)=>{
        if(await this[$semaphore].try_acquire_for(1000)){
            try{
                await next();
            }finally {
                this[$semaphore].release();
            }
        }else{
            throw new Error('server busy');
        }
    }
}
chacent commented 4 years ago

sorry for that i don't know how to format the code

samchon commented 4 years ago

In tstl@2.4.7, I've tried your 1st code like below and it seems no error.

import { Semaphore, randint, sleep_for } from "tstl";

async function main(): Promise<void>
{
    let s: Semaphore = new Semaphore(4);
    let minimum: number = 500;
    let maximum: number = 4000;

    while(true)
    {
        let waitTime: number = Date.now();
        let success: boolean = await s.try_acquire_for(1000);
        waitTime = Date.now() - waitTime;

        if (success === true)
        {
            (async () =>
            {
                let sleepTime: number = randint(minimum, maximum);
                console.log("success, sleep_for", sleepTime, "ms after", waitTime, "ms");

                await sleep_for(sleepTime);
                await s.release();
            })();
        }
        else
            console.log("failed after", waitTime, "ms");
    }
}
main();
success, sleep_for 2490 ms after 1 ms
success, sleep_for 1140 ms after 0 ms
success, sleep_for 3132 ms after 0 ms
success, sleep_for 1355 ms after 0 ms
failed after 1011 ms
success, sleep_for 1760 ms after 139 ms
success, sleep_for 3712 ms after 217 ms
failed after 1011 ms
success, sleep_for 2664 ms after 108 ms
success, sleep_for 3819 ms after 421 ms
success, sleep_for 3290 ms after 231 ms
failed after 1007 ms
success, sleep_for 3995 ms after 931 ms
success, sleep_for 3465 ms after 76 ms
failed after 1009 ms
success, sleep_for 2902 ms after 263 ms
success, sleep_for 3782 ms after 294 ms
failed after 1008 ms
success, sleep_for 3677 ms after 900 ms
success, sleep_for 2255 ms after 450 ms
success, sleep_for 867 ms after 246 ms
success, sleep_for 3709 ms after 870 ms
success, sleep_for 2970 ms after 308 ms
chacent commented 4 years ago

yes,i found no error when test,and this is my test code.but when actual project it occors.now i use acquire instead. i will focus with this later

samchon commented 4 years ago

I'd found a mistaken code that can occur a runtime error in the Semaphore._Cancel() method:

https://github.com/samchon/tstl/blob/228e2471f1581c47df72990127543b9fc4cae098/src/thread/Semaphore.ts#L145-L146

Therefore, I've patched the tstl to 2.4.8 to fix the code like below:

https://github.com/samchon/tstl/blob/4be6620f100179f5639216bc85bfe7bfc55588cf/src/thread/Semaphore.ts#L140-L141

Anyway, looking at your 2nd code, it seems @chacent, you're using the nestjs framework, right? I'll test similar code by creating a test web-server project using the nestjs at next weekend, may Saturday. Until the next wekkend, I hope @chacent you to upgrade your project to using the new tstl@2.4.8 and tell me if same error occurs.

chacent commented 4 years ago

Thank you. I'm sure this problem has no relation with the library I used. If I have time to try to find another piece of code that I made a mistake last time, it must be a problem of counting logic, which may be caused by reentry

chacent commented 4 years ago

and maybe your patch code works, i'll try tomorrow

chacent commented 4 years ago

my framework used is koa,just ignore it,thanks