-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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 deadlock in OvercommitTracker #34591
Conversation
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.
Fix required for release
@@ -8,10 +8,11 @@ using namespace std::chrono_literals; | |||
|
|||
constexpr std::chrono::microseconds ZERO_MICROSEC = 0us; | |||
|
|||
OvercommitTracker::OvercommitTracker() | |||
OvercommitTracker::OvercommitTracker(std::mutex & global_mutex_) |
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.
global_mutex
is unclear name -- what does it mean? We have a lot of "global" objects.
@@ -22,13 +23,15 @@ void OvercommitTracker::setMaxWaitTime(UInt64 wait_time) | |||
|
|||
bool OvercommitTracker::needToStopQuery(MemoryTracker * tracker) | |||
{ | |||
std::unique_lock<std::mutex> global_lock(global_mutex); |
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.
Why we have to use this order? Need description.
std::unique_lock<std::mutex> lk(overcommit_m); | ||
|
||
if (max_wait_time == ZERO_MICROSEC) | ||
return true; | ||
|
||
pickQueryToExclude(); | ||
assert(cancelation_state == QueryCancelationState::RUNNING); | ||
global_lock.unlock(); |
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.
Why we unlocking here? Maybe it's ok to lock inside pickQueryToExclude
?
@@ -87,7 +87,7 @@ struct OvercommitTracker : boost::noncopyable | |||
} | |||
} | |||
|
|||
friend struct BlockQueryIfMemoryLimit; | |||
std::mutex & global_mutex; |
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.
Comment.
@@ -127,29 +125,3 @@ struct GlobalOvercommitTracker : OvercommitTracker | |||
DB::ProcessList * process_list; | |||
Poco::Logger * logger = &Poco::Logger::get("GlobalOvercommitTracker"); | |||
}; | |||
|
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.
We don't need this anymore -- great!
@@ -98,7 +98,7 @@ namespace DB | |||
|
|||
struct UserOvercommitTracker : OvercommitTracker | |||
{ | |||
explicit UserOvercommitTracker(DB::ProcessListForUser * user_process_list_); | |||
explicit UserOvercommitTracker(DB::ProcessList * process_list, DB::ProcessListForUser * user_process_list_); |
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.
redundant explicit
Changelog category (leave one):