Skip to content

Commit

Permalink
Fix potential deadlock in setBinLabel.
Browse files Browse the repository at this point in the history
This one only triggers in the error path, because getFullname tries to
take a lock that we already hold.

However, there is another opportunity for a deadlock via edm::Log*
(which probably takes a lock), and the per ME-locks which could be taken
before or after edm::Log* is called. I reviewed all usages of edm::Log*
in DQMServices/Core and did not see anything problematic.

Note that this is the only usage of edm::Log* in MonitorElement, and it
now avoids taking multiple locks at the same time, so there should not
be any more problems of this type.
  • Loading branch information
schneiml committed Feb 11, 2020
1 parent 7bf830b commit e2a9965
Showing 1 changed file with 18 additions and 12 deletions.
30 changes: 18 additions & 12 deletions DQMServices/Core/src/MonitorElement.cc
Original file line number Diff line number Diff line change
Expand Up @@ -721,18 +721,24 @@ namespace dqm::impl {

/// set bin label for x, y or z axis (axis=1, 2, 3 respectively)
void MonitorElement::setBinLabel(int bin, const std::string &label, int axis /* = 1 */) {
auto access = this->accessMut();
update();
if (getAxis(access, __PRETTY_FUNCTION__, axis)->GetNbins() >= bin) {
getAxis(access, __PRETTY_FUNCTION__, axis)->SetBinLabel(bin, label.c_str());
} else {
#if WITHOUT_CMS_FRAMEWORK
std::cout
#else
edm::LogWarning("MonitorElement")
#endif
<< "*** MonitorElement: WARNING:"
<< "setBinLabel: attempting to set label of non-existent bin number for ME: " << getFullname() << " \n";
bool fail = false;
{
auto access = this->accessMut();
update();
if (getAxis(access, __PRETTY_FUNCTION__, axis)->GetNbins() >= bin) {
getAxis(access, __PRETTY_FUNCTION__, axis)->SetBinLabel(bin, label.c_str());
} else {
fail = true;
}
}
// do this with the ME lock released to prevent a deadlock
if (fail) {
// this also takes the lock, make sure to release it before going to edm
// (which might take more locks)
auto name = getFullname();
edm::LogWarning("MonitorElement") << "*** MonitorElement: WARNING:"
<< "setBinLabel: attempting to set label of non-existent bin number for ME: "
<< name << " \n";
}
}

Expand Down

0 comments on commit e2a9965

Please sign in to comment.