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

Fix data-race between flush() and startup() in StorageBuffer #29930

Merged
merged 1 commit into from
Oct 10, 2021

Conversation

azat
Copy link
Collaborator

@azat azat commented Oct 9, 2021

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix data-race between flush() and startup() in StorageBuffer

Detailed description / Documentation draft:
Stress tests found 1, TSan report:

================== WARNING: ThreadSanitizer: data race (pid=485) Read of size 8 at 0x7b5001280bd8 by thread T567 (mutexes: write M612061890855345680): 1 std::__1::shared_ptr::operator bool() const obj-x86_64-linux-gnu/../contrib/libcxx/include/memory:2851:62 (clickhouse+0x159140a6) 2 bool std::__1::operator!=() obj-x86_64-linux-gnu/../contrib/libcxx/include/memory:3447:30 (clickhouse+0x159140a6) 3 DB::BackgroundSchedulePoolTaskHolder::operator bool() const obj-x86_64-linux-gnu/../src/Core/BackgroundSchedulePool.h:164:46 (clickhouse+0x159140a6) 4 DB::StorageBuffer::flush() obj-x86_64-linux-gnu/../src/Storages/StorageBuffer.cpp:675:10 (clickhouse+0x159140a6)
  Previous write of size 8 at 0x7b5001280bd8 by thread T586 (mutexes: write M191819750614415520):
    2 std::__1::shared_ptr<DB::BackgroundSchedulePoolTaskInfo>::operator=(std::__1::shared_ptr<DB::BackgroundSchedulePoolTaskInfo>&&) obj-x86_64-linux-gnu/../contrib/libcxx/include/memory:3243:34 (clickhouse+0x15913e22)
    3 DB::BackgroundSchedulePoolTaskHolder::operator=() obj-x86_64-linux-gnu/../src/Core/BackgroundSchedulePool.h:156:110 (clickhouse+0x15913e22)
    4 DB::StorageBuffer::startup() obj-x86_64-linux-gnu/../src/Storages/StorageBuffer.cpp:668:18 (clickhouse+0x15913e22)
    5 DB::InterpreterCreateQuery::doCreateTable() obj-x86_64-linux-gnu/../src/Interpreters/InterpreterCreateQuery.cpp:1092:10 (clickhouse+0x149bef7b)
    6 DB::InterpreterCreateQuery::createTable() obj-x86_64-linux-gnu/../src/Interpreters/InterpreterCreateQuery.cpp:952:20 (clickhouse+0x149ba9f5)
    7 DB::InterpreterCreateQuery::execute() obj-x86_64-linux-gnu/../src/Interpreters/InterpreterCreateQuery.cpp:1302:16 (clickhouse+0x149c1086)

Fixes: #29416

@robot-clickhouse robot-clickhouse added pr-bugfix Pull request with bugfix, not backported by default pr-improvement Pull request with some product improvements and removed pr-bugfix Pull request with bugfix, not backported by default labels Oct 9, 2021
Stress tests found [1], TSan report:

    ==================
    WARNING: ThreadSanitizer: data race (pid=485)
      Read of size 8 at 0x7b5001280bd8 by thread T567 (mutexes: write M612061890855345680):
        1 std::__1::shared_ptr<DB::BackgroundSchedulePoolTaskInfo>::operator bool() const obj-x86_64-linux-gnu/../contrib/libcxx/include/memory:2851:62 (clickhouse+0x159140a6)
        2 bool std::__1::operator!=<DB::BackgroundSchedulePoolTaskInfo>() obj-x86_64-linux-gnu/../contrib/libcxx/include/memory:3447:30 (clickhouse+0x159140a6)
        3 DB::BackgroundSchedulePoolTaskHolder::operator bool() const obj-x86_64-linux-gnu/../src/Core/BackgroundSchedulePool.h:164:46 (clickhouse+0x159140a6)
        4 DB::StorageBuffer::flush() obj-x86_64-linux-gnu/../src/Storages/StorageBuffer.cpp:675:10 (clickhouse+0x159140a6)

      Previous write of size 8 at 0x7b5001280bd8 by thread T586 (mutexes: write M191819750614415520):
        2 std::__1::shared_ptr<DB::BackgroundSchedulePoolTaskInfo>::operator=(std::__1::shared_ptr<DB::BackgroundSchedulePoolTaskInfo>&&) obj-x86_64-linux-gnu/../contrib/libcxx/include/memory:3243:34 (clickhouse+0x15913e22)
        3 DB::BackgroundSchedulePoolTaskHolder::operator=() obj-x86_64-linux-gnu/../src/Core/BackgroundSchedulePool.h:156:110 (clickhouse+0x15913e22)
        4 DB::StorageBuffer::startup() obj-x86_64-linux-gnu/../src/Storages/StorageBuffer.cpp:668:18 (clickhouse+0x15913e22)
        5 DB::InterpreterCreateQuery::doCreateTable() obj-x86_64-linux-gnu/../src/Interpreters/InterpreterCreateQuery.cpp:1092:10 (clickhouse+0x149bef7b)
        6 DB::InterpreterCreateQuery::createTable() obj-x86_64-linux-gnu/../src/Interpreters/InterpreterCreateQuery.cpp:952:20 (clickhouse+0x149ba9f5)
        7 DB::InterpreterCreateQuery::execute() obj-x86_64-linux-gnu/../src/Interpreters/InterpreterCreateQuery.cpp:1302:16 (clickhouse+0x149c1086)

  [1]: https://clickhouse-test-reports.s3.yandex.net/0/1c9778603ff49563d1d3d0d357de0608167e504d/stress_test_(thread).html

Fixes: ClickHouse#29416
Copy link
Member

@alexey-milovidov alexey-milovidov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

@alexey-milovidov alexey-milovidov self-assigned this Oct 10, 2021
@alexey-milovidov alexey-milovidov merged commit 0adbcd5 into ClickHouse:master Oct 10, 2021
@azat azat deleted the buffer-fix-data-race branch October 10, 2021 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-improvement Pull request with some product improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ThreadSanitizer: data race StorageBuffer::flush
3 participants