Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] InstallGameService.cs中部分代码线程安全问题(竞争执行Bug) #1082

Closed
1 task done
iamscottxu opened this issue Sep 8, 2024 · 4 comments
Closed
1 task done

Comments

@iamscottxu
Copy link
Contributor

iamscottxu commented Sep 8, 2024

Checklist

  • My issue was not mentioned by others, and it is not a duplicate issue.

Description

InstallGameService.cs:

image image

红框框起来的代码有线程安全问题(竞争执行Bug)。具体来说是这样写的话读取与自增或自减不是原子操作,在特定条件下会造成判断条件在多个线程中多次触发,可能会造程序行为异常。

建议改为以下代码:

var concurrentExecuteThreadCount = Interlocked.Increment(ref _concurrentExecuteThreadCount);
if (concurrentExecuteThreadCount > Environment.ProcessorCount)
{
    return;
}
var concurrentExecuteThreadCount = Interlocked.Decrement(ref _concurrentExecuteThreadCount);
if (concurrentExecuteThreadCount == 0)
{
    CurrentTaskFinished();
}

且与 #1083 问题一起修改后,这些代码将会变为冗余代码。

Reproduction Steps

No response

Expected Behavior

No response

Screenshots

No response

Starward Version

main分支:37253b8

Windows Version

与操作系统版本无关

Log

No response

Additional Context

已经提pr #1084 修复。

@iamscottxu
Copy link
Contributor Author

#1084

@Scighost
Copy link
Owner

Scighost commented Sep 8, 2024

我觉得这个办法不行,假设在 Interlocked.Incrementif (concurrentExecuteThreadCount > Environment.ProcessorCount) 之间有其他线程修改了 _concurrentExecuteThreadCount 的值,那么临时变量 concurrentExecuteThreadCount 的值就不等于真实值。

@Scighost Scighost added Area: Others and removed triage Issue needs to be triaged labels Sep 8, 2024
@iamscottxu
Copy link
Contributor Author

iamscottxu commented Sep 8, 2024

@Scighost 这里本身就不需要知道其在判断逻辑执行时的真实值,而是要得到自增自减操作后的值,也就是说自增自减读取必须是一个完整的原子操作。就拿以下例子来说,我们不需要关心是哪一个线程获得了11这个值,而是要保证只有一个线程能获取到11这个值,因为这些线程无论是先启动的还是后启动的,都是等价的。所以说我们要确保的恰恰就是这两个语句中间其他线程对原始变量的改变不会影响到后面判断时的读取操作,否则就会导致多个线程读取到相同的值,进而多个线程都能通过逻辑判断,这样本来不该多次执行的代码就被多次执行了。

简单做了一个复现例子来说明这个问题,这里在两行代码之间引入延迟来提高复现概率。

期望:只输出一次11。
image
image

由上图可知,如果自增自减和读取不是原子操作的话,导致每个Task都输出了一次11,如果换成return,可能会造成期望的线程数与实际上的线程数不符。如果换成其他非线程安全的方法,可能会造成此非线程安全方法被多个线程多次执行,导致异常行为。

var _taskCount = 0;

async Task Test()
{
    Interlocked.Increment(ref _taskCount);
    await Task.Delay(500);
    if (_taskCount > 10)
    {
        Console.WriteLine(_taskCount);
    }
    await Task.Delay(1000);
}


_taskCount = 0;
var tasks = new Task[11];
for (var i = 0; i < 11; i++)
{
    tasks[i] = Test();
}
await Task.WhenAll(tasks);
Console.WriteLine("Finish");

下面再附一个stackoverflow中类似Bug的案例:
https://stackoverflow.com/questions/42137152/interlocked-increment-and-return-of-incremented-value

@iamscottxu iamscottxu changed the title [Bug] InstallGameService.cs中部分代码线程安全问题 [Bug] InstallGameService.cs中部分代码线程安全问题(竞争执行Bug) Sep 8, 2024
@Scighost
Copy link
Owner

谢谢你指出我的错误

Scighost pushed a commit that referenced this issue Sep 15, 2024
…tiple times by multiple threads (#1084)

* fixed the bug that method `CurrentTaskFinished` was executed multiple times by multiple threads (#1083) (#1082)

Signed-off-by: OsakaRuma <[email protected]>

* handling exception, deleted voice code

Signed-off-by: OsakaRuma <[email protected]>

* fixed typo of variable name

Signed-off-by: OsakaRuma <[email protected]>

* fixed typo in variable name

Signed-off-by: OsakaRuma <[email protected]>

* fixed a bug where only the first step of the task could be run if there were multiple steps of the task when installing the game

Signed-off-by: OsakaRuma <[email protected]>

---------

Signed-off-by: OsakaRuma <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants