andreacavalli/scylla-jmx
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

APIBuilder: Remove RW-lock in JMX server repository wrapper

Browse Source 
This is a seemingly pointless change. The RW-lock code is 100%
correct (afaict), yet we've seen repeated cases of test runs
hanging in JMX query because this lock is seemingly left held
by what seems to be the reaper task.

There is no explanation for this, no sign of exceptions/errors
that could explain the lock being broken. Nor any known JDK/JVM
bugs.

Yet, in tests, it seems that replacing the lock with a more
coarse, yet proven, synchronized, fixes the issue. So there.

I officially hate this patch, and it should not exist.
This commit is contained in:
Calle Wilund 2021-03-03 12:18:33 +00:00 committed by Avi Kivity
parent 8073af6e06
commit c2fc96be71
1 changed files with 8 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

33
src/main/java/com/scylladb/jmx/utils/APIBuilder.java
View File

@ -66,7 +66,7 @@ public class APIBuilder extends MBeanServerBuilder {
private final Repository wrapped; private final Repository wrapped;
private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(); private final Object lock = new Object();
private final Map<TableMetricParams, DynamicMBean> tableMBeans = new HashMap<>(); private final Map<TableMetricParams, DynamicMBean> tableMBeans = new HashMap<>();
@ -93,11 +93,8 @@ public class APIBuilder extends MBeanServerBuilder {
if (!isTableMetricName(name)) { if (!isTableMetricName(name)) {
return wrapped.contains(name); return wrapped.contains(name);
} else { } else {
lock.readLock().lock(); synchronized(lock) {
try {
return tableMBeans.containsKey(new TableMetricParams(name)); return tableMBeans.containsKey(new TableMetricParams(name));
} finally {
lock.readLock().unlock();
} }
} }
} }
@ -116,23 +113,20 @@ public class APIBuilder extends MBeanServerBuilder {
@Override @Override
public Integer getCount() { public Integer getCount() {
lock.readLock().lock(); synchronized(lock) {
try {
return wrapped.getCount() + tableMBeans.size(); return wrapped.getCount() + tableMBeans.size();
} finally {
lock.readLock().unlock();
} }
} }
@Override @Override
public void addMBean(final DynamicMBean bean, final ObjectName name, final RegistrationContext ctx) public void addMBean(final DynamicMBean bean, final ObjectName name, final RegistrationContext ctx)
throws InstanceAlreadyExistsException { throws InstanceAlreadyExistsException {
System.out.println(name);
if (!isTableMetricName(name)) { if (!isTableMetricName(name)) {
wrapped.addMBean(bean, name, ctx); wrapped.addMBean(bean, name, ctx);
} else { } else {
final TableMetricParams key = new TableMetricParams(name); final TableMetricParams key = new TableMetricParams(name);
lock.writeLock().lock(); synchronized(lock) {
try {
if (tableMBeans.containsKey(key)) { if (tableMBeans.containsKey(key)) {
throw new InstanceAlreadyExistsException(name.toString()); throw new InstanceAlreadyExistsException(name.toString());
} }
@ -145,8 +139,6 @@ public class APIBuilder extends MBeanServerBuilder {
} catch (RuntimeException x) { } catch (RuntimeException x) {
throw new RuntimeOperationsException(x); throw new RuntimeOperationsException(x);
} }
} finally {
lock.writeLock().unlock();
} }
} }
} }
@ -157,8 +149,7 @@ public class APIBuilder extends MBeanServerBuilder {
wrapped.remove(name, ctx); wrapped.remove(name, ctx);
} else { } else {
final TableMetricParams key = new TableMetricParams(name); final TableMetricParams key = new TableMetricParams(name);
lock.writeLock().lock(); synchronized(lock) {
try {
if (tableMBeans.remove(key) == null) { if (tableMBeans.remove(key) == null) {
throw new InstanceNotFoundException(name.toString()); throw new InstanceNotFoundException(name.toString());
} }
@ -171,8 +162,6 @@ public class APIBuilder extends MBeanServerBuilder {
} catch (Exception x) { } catch (Exception x) {
logger.log(SEVERE, "Unexpected error.", x); logger.log(SEVERE, "Unexpected error.", x);
} }
} finally {
lock.writeLock().lock();
} }
} }
} }
@ -182,11 +171,8 @@ public class APIBuilder extends MBeanServerBuilder {
if (!isTableMetricName(name)) { if (!isTableMetricName(name)) {
return wrapped.retrieve(name); return wrapped.retrieve(name);
} else { } else {
lock.readLock().lock(); synchronized(lock) {
try {
return tableMBeans.get(new TableMetricParams(name)); return tableMBeans.get(new TableMetricParams(name));
} finally {
lock.readLock().unlock();
} }
} }
} }
@ -229,8 +215,7 @@ public class APIBuilder extends MBeanServerBuilder {
name = pattern; name = pattern;
} }
lock.readLock().lock(); synchronized(lock) {
try {
// If pattern is not a pattern, retrieve this mbean ! // If pattern is not a pattern, retrieve this mbean !
if (!name.isPattern() && isTableMetricName(name)) { if (!name.isPattern() && isTableMetricName(name)) {
final DynamicMBean bean = tableMBeans.get(new TableMetricParams(name)); final DynamicMBean bean = tableMBeans.get(new TableMetricParams(name));
@ -286,8 +271,6 @@ public class APIBuilder extends MBeanServerBuilder {
addAllMatching(res, namePattern); addAllMatching(res, namePattern);
} }
} }
} finally {
lock.readLock().unlock();
} }
return res; return res;
} }