[java] Update AvoidSynchronizedAtMethodLevel message to mention ReentrantLock, new rule AvoidSynchronizedStatement (#5175)

Merge pull request #5175 from chonton:ReentrantLock

.all-contributorsrc

@@ -1929,7 +1929,8 @@
       "avatar_url": "https://avatars.githubusercontent.com/u/1444125?v=4",
       "profile": "http://www.linkedin.com/in/chonton",
       "contributions": [
-        "bug"
+        "bug",
+        "code"
       ]
     },
     {

docs/pages/pmd/projectdocs/credits.md

@@ -171,7 +171,7 @@ Thanks goes to these wonderful people ([emoji key](https://allcontributors.org/d
       <td align="center" valign="top" width="14.28%"><a href="https://c-otto.de/"><img src="https://avatars.githubusercontent.com/u/344948?v=4?s=100" width="100px;" alt="Carsten Otto"/><br /><sub><b>Carsten Otto</b></sub></a><br /><a href="https://github.com/pmd/pmd/issues?q=author%3AC-Otto" title="Bug reports">🐛</a></td>
       <td align="center" valign="top" width="14.28%"><a href="https://github.com/HoushCE29"><img src="https://avatars.githubusercontent.com/u/22387324?v=4?s=100" width="100px;" alt="Charlie Housh"/><br /><sub><b>Charlie Housh</b></sub></a><br /><a href="https://github.com/pmd/pmd/issues?q=author%3AHoushCE29" title="Bug reports">🐛</a></td>
       <td align="center" valign="top" width="14.28%"><a href="https://github.com/ChuckJonas"><img src="https://avatars.githubusercontent.com/u/5217568?v=4?s=100" width="100px;" alt="Charlie Jonas"/><br /><sub><b>Charlie Jonas</b></sub></a><br /><a href="https://github.com/pmd/pmd/issues?q=author%3AChuckJonas" title="Bug reports">🐛</a></td>
-      <td align="center" valign="top" width="14.28%"><a href="http://www.linkedin.com/in/chonton"><img src="https://avatars.githubusercontent.com/u/1444125?v=4?s=100" width="100px;" alt="Chas Honton"/><br /><sub><b>Chas Honton</b></sub></a><br /><a href="https://github.com/pmd/pmd/issues?q=author%3Achonton" title="Bug reports">🐛</a></td>
+      <td align="center" valign="top" width="14.28%"><a href="http://www.linkedin.com/in/chonton"><img src="https://avatars.githubusercontent.com/u/1444125?v=4?s=100" width="100px;" alt="Chas Honton"/><br /><sub><b>Chas Honton</b></sub></a><br /><a href="https://github.com/pmd/pmd/issues?q=author%3Achonton" title="Bug reports">🐛</a> <a href="https://github.com/pmd/pmd/commits?author=chonton" title="Code">💻</a></td>
       <td align="center" valign="top" width="14.28%"><a href="https://github.com/willamette"><img src="https://avatars.githubusercontent.com/u/1435016?v=4?s=100" width="100px;" alt="Chen Yang"/><br /><sub><b>Chen Yang</b></sub></a><br /><a href="https://github.com/pmd/pmd/issues?q=author%3Awillamette" title="Bug reports">🐛</a></td>
       <td align="center" valign="top" width="14.28%"><a href="https://github.com/bashagithub"><img src="https://avatars.githubusercontent.com/u/25314601?v=4?s=100" width="100px;" alt="Chotu"/><br /><sub><b>Chotu</b></sub></a><br /><a href="https://github.com/pmd/pmd/issues?q=author%3Abashagithub" title="Bug reports">🐛</a></td>
       <td align="center" valign="top" width="14.28%"><a href="https://github.com/tophersmith"><img src="https://avatars.githubusercontent.com/u/812876?v=4?s=100" width="100px;" alt="Chris Smith"/><br /><sub><b>Chris Smith</b></sub></a><br /><a href="https://github.com/pmd/pmd/issues?q=author%3Atophersmith" title="Bug reports">🐛</a></td>

docs/pages/release_notes.md

@@ -14,6 +14,11 @@ This is a {{ site.pmd.release_type }} release.
 
 ### 🚀 New and noteworthy
 
+### 🌟 New and changed rules
+#### New Rules
+* The new Java rule {%rule java/multithreading/AvoidSynchronizedStatement %} finds synchronization blocks that
+  could cause performance issues with virtual threads due to pinning.
+
 ### 🐛 Fixed Issues
 * apex-performance
   * [#5139](https://github.com/pmd/pmd/issues/5139): \[apex] OperationWithHighCostInLoop: false negative for triggers
@@ -26,6 +31,8 @@ This is a {{ site.pmd.release_type }} release.
   * [#5151](https://github.com/pmd/pmd/issues/5151): \[java] GuardLogStatement: Should not need to guard parameterized log messages where the replacement arg is a constant from another class
   * [#5152](https://github.com/pmd/pmd/issues/5152): \[java] GuardLogStatement: Should not need to guard parameterized log messages where the replacement arg is "this"
   * [#5153](https://github.com/pmd/pmd/issues/5153): \[java] GuardLogStatement: Should not need to guard parameterized log messages where the replacement arg is an array element
+* java-multithreading
+  * [#5175](https://github.com/pmd/pmd/issues/5175): \[java] Update AvoidSynchronizedAtMethodLevel message to mention ReentrantLock, new rule AvoidSynchronizedStatement
 * plsql
   * [#5125](https://github.com/pmd/pmd/pull/5125): \[plsql] Improve merge statement (order of merge insert/update flexible, allow prefixes in column names)
 * plsql-bestpractices
@@ -48,6 +55,7 @@ This is a {{ site.pmd.release_type }} release.
 
 ### ✨ External Contributions
 * [#5125](https://github.com/pmd/pmd/pull/5125): \[plsql] Improve merge statement (order of merge insert/update flexible, allow prefixes in column names) - [Arjen Duursma](https://github.com/duursma) (@duursma)
+* [#5175](https://github.com/pmd/pmd/pull/5175): \[java] Update AvoidSynchronizedAtMethodLevel message to mention ReentrantLock, new rule AvoidSynchronizedStatement - [Chas Honton](https://github.com/chonton) (@chonton)
 
 {% endtocmaker %}
 

pmd-java/src/main/resources/category/java/multithreading.xml

@@ -12,13 +12,13 @@ Rules that flag issues when dealing with multiple threads of execution.
     <rule name="AvoidSynchronizedAtMethodLevel"
           language="java"
           since="3.0"
-          message="Use block level rather than method level synchronization"
+          message="Use block level locking rather than method level synchronization"
           class="net.sourceforge.pmd.lang.rule.xpath.XPathRule"
           externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_multithreading.html#avoidsynchronizedatmethodlevel">
         <description>
-Method-level synchronization can cause problems when new code is added to the method.
+Method-level synchronization will pin virtual threads and can cause performance problems. Additionally, it can cause
-Block-level synchronization helps to ensure that only the code that needs synchronization
+problems when new code is added to the method.  Block-level ReentrantLock helps to ensure that only the code that
-gets it.
+needs mutual exclusion will be locked.
         </description>
         <priority>3</priority>
         <properties>
@@ -41,13 +41,18 @@ public class Foo {
         // ...
     }
     // Prefer this:
+    Lock instanceLock = new ReentrantLock();
+
     void bar() {
         // code, that doesn't need synchronization
         // ...
-        synchronized(this) {
+        try {
+            instanceLock.lock();  // or instanceLock.tryLock(long time, TimeUnit unit)
             if (!sharedData.has("bar")) {
                 sharedData.add("bar");
             }
+        } finally {
+            instanceLock.unlock();
         }
         // more code, that doesn't need synchronization
         // ...
@@ -58,11 +63,16 @@ public class Foo {
     }
 
     // Prefer this:
+    private static Lock CLASS_LOCK = new ReentrantLock();
+
     static void barStatic() {
         // code, that doesn't need synchronization
         // ...
-        synchronized(Foo.class) {
+        try {
+            CLS_LOCK.lock();
             // code, that requires synchronization
+        } finally {
+            CLS_LOCK.unlock();
         }
         // more code, that doesn't need synchronization
         // ...
@@ -72,6 +82,50 @@ public class Foo {
         </example>
     </rule>
 
+  <rule name="AvoidSynchronizedStatement"
+        language="java"
+        since="7.5.0"
+        message="Use ReentrantLock rather than synchronization"
+        class="net.sourceforge.pmd.lang.rule.xpath.XPathRule"
+        externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_multithreading.html#avoidsynchronizedstatement">
+    <description>
+      Synchronization will pin virtual threads and can cause performance problems.
+    </description>
+    <priority>3</priority>
+    <properties>
+      <property name="xpath">
+        <value>//SynchronizedStatement</value>
+      </property>
+    </properties>
+    <example>
+      <![CDATA[
+public class Foo {
+    // Try to avoid this:
+    void foo() {
+        // code that doesn't need mutual exclusion
+        synchronized(this) {
+            // code that requires mutual exclusion
+        }
+        // more code that doesn't need mutual exclusion
+    }
+    // Prefer this:
+    Lock instanceLock = new ReentrantLock();
+
+    void foo() {
+        // code that doesn't need mutual exclusion
+        try {
+            instanceLock.lock();  // or instanceLock.tryLock(long time, TimeUnit unit)
+            // code that requires mutual exclusion
+        } finally {
+            instanceLock.unlock();
+        }
+        // more code that doesn't need mutual exclusion
+    }
+}
+]]>
+    </example>
+  </rule>
+
     <rule name="AvoidThreadGroup"
           language="java"
           since="3.6"

pmd-java/src/main/resources/rulesets/java/quickstart.xml

@@ -279,6 +279,7 @@
 
 
     <!-- <rule ref="category/java/multithreading.xml/AvoidSynchronizedAtMethodLevel" /> -->
+    <!-- <rule ref="category/java/multithreading.xml/AvoidSynchronizedStatement" /> -->
     <rule ref="category/java/multithreading.xml/AvoidThreadGroup"/>
     <rule ref="category/java/multithreading.xml/AvoidUsingVolatile"/>
     <!-- <rule ref="category/java/multithreading.xml/DoNotUseThreads" /> -->

pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/multithreading/AvoidSynchronizedStatementTest.java

@@ -0,0 +1,11 @@
+/**
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.rule.multithreading;
+
+import net.sourceforge.pmd.test.PmdRuleTst;
+
+class AvoidSynchronizedStatementTest extends PmdRuleTst {
+    // no additional unit tests
+}

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/multithreading/xml/AvoidSynchronizedStatement.xml

@@ -0,0 +1,43 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<test-data
+    xmlns="http://pmd.sourceforge.net/rule-tests"
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd">
+
+    <test-code>
+        <description>Synchronized block in instance method</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Foo {
+    void foo () {
+        synchronized(mutex) {}
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Synchronized block in static methods</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Test {
+    public static void foo() {
+        synchronized(Test.class) {
+            // only a block is synchronized on Test.class
+        }
+    }
+}
+        ]]></code>
+    </test-code>
+    
+    <test-code>
+        <description>synchronized methods are not flagged - we have a separate rule AvoidSynchronizedAtMethodLevel for that</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Test {
+    public synchronized void foo() {}
+    public static synchronized void fooStatic() {}
+}
+        ]]></code>
+    </test-code>
+</test-data>