APIBuilder: Remove RW-lock in JMX server repository wrapper

[elcallio]

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.

[haaawk]

Interesting.

[penberg]

@elcallio didn't the original code have a missing unlock. Or, rather, accidental double lock:

diff --git a/src/main/java/com/scylladb/jmx/utils/APIBuilder.java b/src/main/java/com/scylladb/jmx/utils/APIBuilder.java
index 5fe3f51..08a9f48 100644
--- a/src/main/java/com/scylladb/jmx/utils/APIBuilder.java
+++ b/src/main/java/com/scylladb/jmx/utils/APIBuilder.java
@@ -172,7 +172,7 @@ public class APIBuilder extends MBeanServerBuilder {
                         logger.log(SEVERE, "Unexpected error.", x);
                     }
                 } finally {
-                    lock.writeLock().lock();
+                    lock.writeLock().unlock();
                 }
             }
         }

[haaawk]

src/main/java/com/scylladb/jmx/utils/APIBuilder.java

good catch @penberg

[elcallio]

@penberg Oooh, my god - the smoking gun and my eyes could not see it - just the matched finallys! I promise, I will never talk smack about finns again! (Maybe just a little)

[elcallio]

I really, really wish that had been noticed earlier....

[elcallio]

@haaawk - if you like, you (or I) can restore the original locking + fix the broken/missing unlock there. But plain sync should be fine really...

[penberg]

I sent a pull request to fix this: https://github.com/scylladb/scylla-jmx/pull/163

[elcallio]

You are so quick and efficient!

[penberg]

@elcallio To be honest, I had already convinced myself that the usage was correct and was searching the JVM bug database, but accidentally stumbled across this... 🤣

[haaawk]

I went through all the lockings as well trying to find out what's wrong and failed as well :D

[elcallio]

I think the conclusion is that we all suck, except @penberg.