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 race on Context::async_insert_queue
#59082
Conversation
This is an automated comment for commit d190ee8 with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page
Successful checks
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has caught my attention as I've been working on the asynchronous queue in #58486
Looks good to me, thanks for the fix!
Just curious, have you considered using std::atomic<std::shared_ptr> instead of locking the mutex?
Why doesn't it have a |
Because it's nearly impossible to provide a user-readable changelog entry for this fix |
AFAIK there's a spinlock inside atomic shared pointer, so there's no big difference. Also, we use this mutex to protect other fields of |
src/Interpreters/Context.cpp
Outdated
std::shared_ptr<AsynchronousInsertQueue> delete_async_insert_queue; | ||
{ | ||
std::lock_guard lock(mutex); | ||
delete_async_insert_queue = std::move(async_insert_queue); | ||
} | ||
delete_async_insert_queue.reset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, and this is also wrong because other threads still can get a raw pointer from getAsynchronousInsertQueue()
and continue using it after reset()
c04fecb
to
1913466
Compare
@@ -218,7 +218,7 @@ AsynchronousInsertQueue::AsynchronousInsertQueue(ContextPtr context_, size_t poo | |||
dump_by_first_update_threads.emplace_back([this, i] { processBatchDeadlines(i); }); | |||
} | |||
|
|||
AsynchronousInsertQueue::~AsynchronousInsertQueue() | |||
void AsynchronousInsertQueue::flushAndShutdown() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please comment that an external entity (Context) is responsible for flushing the queue since it's not happening in the destructor anymore.
AST fuzzer (msan) - #61771 |
Changelog category (leave one):
https://s3.amazonaws.com/clickhouse-test-reports/39507/fe1e326701b0566881dc7d51793125a911faa07e/integration_tests__tsan__[4_6].html
Introduced in #53547