-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Data race in AsyncLogging #288
Comments
严格地说,是有 data race。改成 atomic 是对的,C++ 98 也可以用 GCC Atomic Builtins。EventLoop::quit_ 同理。 |
但是这里的 data race 不会影响程序的正确性,所以在考虑性能的情况下是可以接受的,可以这样理解吗? |
如果严格按照 C++ 标准来说,会影响正确性。不过在 X86 Linux 下用 GCC / clang 编译出来的程序到目前为止还没有出错。 |
不好意思可能是我没表达清楚,我的意思是说是否考虑到 atomic 或者 mutex(EventLoop::quit_ 的情况只用 atomic 好像还是不能保证线程安全)的开销,所以才没有做出修改? |
No.
How ? |
第一点看到您已经修复了;第二点我是这样想的: |
如果同一个线程依次调用 quit() 和 loop(),会死锁。 |
对的,是我之前考虑不周了。我又想了一下,其实 quit() 的设计初衷之一应该是支持 quit() 之后再调用 loop() 的吧(否则在 loop() 中也就不必加 108 这一行了)。在这种情况下我能想到的最好的解决方案就是忽略在 loop() 开始之前的 quit() 调用,也就是现在这种处理方法了。 |
108 这一行原本是为了支持 loop() -> quit() -> 再次 loop()。不过好像没有人这么用。 |
是的。不过我觉得这样会有一些语义上的问题吧,比如在 loop1 -> quit1 -> quit2 -> loop2 这个过程中,我认为没有办法分清 quit2 的调用意图是结束 loop1 还是结束 loop2,所以我觉得在两次 loop 之间(一次结束之后另一次开始之前,以及第一次 loop 开始之前)的 quit 直接忽略掉比较好一点。这一点您怎么想呢? |
陈老师你好,最近在学习 AsyncLogging 部分的时候,发现一个可能的潜在 data race 问题:
在 threadFunc 中 while (running_) 与 stop 中的 running_ = false 语句可能会存在对 running_ 访问的 data race,改成 atomic<bool> 应该可以解决这个问题。
当然如果程序永远在循环中不退出的话是不会有问题的,不知道这个地方是否有必要修改一下?
The text was updated successfully, but these errors were encountered: