[JSPWIKI-1178] Address potential deadlock with JDK 21 Virtual Threads. #307

arturobernalg · 2023-09-11T20:58:54Z

Refactored synchronized blocks/methods containing blocking operations to prevent potential deadlocks introduced by the upcoming Virtual Threads feature in JDK 21.

juanpablo-santos · 2023-10-02T21:12:36Z

Hi,

interesting PR! :-) My only comment is that I'd extract the lock behaviour on a utility class something similar to (pseudocode, surely doesn't even compile):

public class Synchronizer {

    public static < T > T synchronize( final ReentrantLock lock, final Supplier< T > supplier ) {
        lock.lock();
	try {
	    return supplier.get();
	} finally {
	    lock.unlock();
	}
    }

}

so code inside synchronized blocks can be passed as a lambda (also this method could be overloaded so it could also receive some Runnable, Function, etc.)

WDYT?

cheers,
juan pablo

arturobernalg · 2023-10-03T16:36:28Z

Hi,

interesting PR! :-) My only comment is that I'd extract the lock behaviour on a utility class something similar to (pseudocode, surely doesn't even compile):
public class Synchronizer {

    public static < T > T synchronize( final ReentrantLock lock, final Supplier< T > supplier ) {
        lock.lock();
	try {
	    return supplier.get();
	} finally {
	    lock.unlock();
	}
    }

}
so code inside synchronized blocks can be passed as a lambda (also this method could be overloaded so it could also receive some Runnable, Function, etc.)

WDYT?

cheers, juan pablo

HI @juanpablo-santos

I agree that encapsulating the lock behavior in a utility class like Synchronizer would offer several advantages, particularly in terms of code Readability and Reusability.

However, this approach has limitations when it comes to working with condition variables and allowing for custom scenarios. Specifically, using a utility class for locking would make it challenging to implement more complex control flows that involve waiting for certain conditions to be met or signaling other threads to proceed.

In essence, while the utility class would make the code cleaner for basic locking and unlocking, it might not be flexible enough to handle advanced locking scenarios that require the use of conditions.

Best regards,

juanpablo-santos · 2023-10-09T09:05:50Z

Hi @arturobernalg !

However, this approach has limitations when it comes to working with condition variables and allowing for custom scenarios. Specifically, using a utility class for locking would make it challenging to implement more complex control flows that involve waiting for certain conditions to be met or signaling other threads to proceed.

In essence, while the utility class would make the code cleaner for basic locking and unlocking, it might not be flexible enough to handle advanced locking scenarios that require the use of conditions.

I agree that more complex scenarios would not fit inside this utility class. However, the scope of the PR is to switch away from synchronized blocks, and for all them we have the same idiom all over the place:

lock.lock();
try {
    doSomething();
} finally {
    lock.unlock();
}

So abstracting it into a common method makes sense to me. If later on we want to refactor, or we want to capture more complex scenarios, we can always move away from the utility synchronize method. The utility/class method focus should be more about synchronizing than locking (hence the names).

WDYT?

arturobernalg · 2023-10-09T16:46:53Z

Hi @arturobernalg !

However, this approach has limitations when it comes to working with condition variables and allowing for custom scenarios. Specifically, using a utility class for locking would make it challenging to implement more complex control flows that involve waiting for certain conditions to be met or signaling other threads to proceed.
In essence, while the utility class would make the code cleaner for basic locking and unlocking, it might not be flexible enough to handle advanced locking scenarios that require the use of conditions.

I agree that more complex scenarios would not fit inside this utility class. However, the scope of the PR is to switch away from synchronized blocks, and for all them we have the same idiom all over the place:
lock.lock();
try {
    doSomething();
} finally {
    lock.unlock();
}
So abstracting it into a common method makes sense to me. If later on we want to refactor, or we want to capture more complex scenarios, we can always move away from the utility synchronize method. The utility/class method focus should be more about synchronizing than locking (hence the names).

WDYT?

HI @juanpablo-santos
It makes sense to start with this abstraction for the sake of code cleanliness and readability.

I'll go ahead and make the changes to incorporate the new method It might take me a couple of days to complete the update, but I'll keep you posted on the progress.

TY

arturobernalg · 2023-10-10T07:07:10Z

The utility/class method focus should be more about synchronizing than locking (hence the names).

Hi @arturobernalg !

However, this approach has limitations when it comes to working with condition variables and allowing for custom scenarios. Specifically, using a utility class for locking would make it challenging to implement more complex control flows that involve waiting for certain conditions to be met or signaling other threads to proceed.
In essence, while the utility class would make the code cleaner for basic locking and unlocking, it might not be flexible enough to handle advanced locking scenarios that require the use of conditions.

I agree that more complex scenarios would not fit inside this utility class. However, the scope of the PR is to switch away from synchronized blocks, and for all them we have the same idiom all over the place:
lock.lock();
try {
    doSomething();
} finally {
    lock.unlock();
}
So abstracting it into a common method makes sense to me. If later on we want to refactor, or we want to capture more complex scenarios, we can always move away from the utility synchronize method. The utility/class method focus should be more about synchronizing than locking (hence the names).
WDYT?
HI @juanpablo-santos It makes sense to start with this abstraction for the sake of code cleanliness and readability.

I'll go ahead and make the changes to incorporate the new method It might take me a couple of days to complete the update, but I'll keep you posted on the progress.

TY

HI @juanpablo-santos

I've just implemented the custom Synchronizer class to manage ReentrantLock synchronization, as initially planned. However, the scope of the changes turned out to be more extensive than I had originally anticipated. As a result, I'm not entirely confident in the current implementation. I had to create several methods to handle all possible use cases, and there were a couple of scenarios that I couldn't address. I'm looking forward to your feedback to determine if this approach is suitable for our use case.

juanpablo-santos

Overall, the PR is in a great shape, congrats!

I've noted several places that might need the use of several locks, so they mimic the current syncing behaviour (I might have missed some, pls take a look overall), and there's a small nitch with how the util module is referenced on pom.xml files, but they're details within the amount of work done here. This should be very close to merge :-)

thanks,
juan pablo

juanpablo-santos · 2023-10-11T08:38:22Z

jspwiki-event/src/main/java/org/apache/wiki/event/WikiEventManager.java

-                // start up any post-instantiation services here
-            }
+        if (c_instance == null) {
+            Synchronizer.synchronize(lock, () -> {


seems that the synchronized blocks on this class do so against different objects: WikiEventManager.class, m_delegates, m_preloadCache, etc., so it would make more sense to use different locks on Synchronizer.synchronize calls?

juanpablo-santos · 2023-10-11T08:40:54Z

jspwiki-main/src/main/java/org/apache/wiki/WatchDog.java

+     *
+     * @see java.util.concurrent.locks.ReentrantLock
+     */
+    private final ReentrantLock lock;


same as noted above, perhaps more locks are needed (?)

juanpablo-santos · 2023-10-11T08:47:44Z

jspwiki-main/src/main/java/org/apache/wiki/WatchDog.java

@@ -302,15 +315,15 @@ private void dumpStackTraceForWatchable() {
     */
    @Override
    public String toString() {
-        synchronized( m_stateStack ) {
+        return Synchronizer.synchronize(lock, () -> {


unrelated to the PR itself, but java.util.Stack is thread-safe (extends Vector) so not sure why this was synced on in the first place... Also, perhaps it could be substituted with a ConcurrentLinkedDeque. Anyway, out of PR's scope.

juanpablo-santos · 2023-10-11T08:49:42Z

jspwiki-main/src/main/java/org/apache/wiki/WikiEngine.java

+     *
+     * @see java.util.concurrent.locks.ReentrantLock
+     */
+    private static final ReentrantLock lock = new ReentrantLock();


again, perhaps several locks are needed

juanpablo-santos · 2023-10-11T08:55:58Z

jspwiki-main/src/main/java/org/apache/wiki/auth/SessionMonitor.java

+     *
+     * @see ReentrantLock
+     */
+    private final ReentrantLock lock;


syncs against m_sessions and at method level, so it should use two locks (?)

juanpablo-santos · 2023-10-11T09:37:54Z

jspwiki-main/src/main/java/org/apache/wiki/workflow/DefaultWorkflowManager.java

@@ -70,6 +82,7 @@ public DefaultWorkflowManager() {
        m_approvers = new ConcurrentHashMap<>();
        m_queue = new DecisionQueue();
        WikiEventEmitter.attach( this );
+      //  this.lock = new ReentrantLock();


maybe delete this line?

juanpablo-santos · 2023-10-11T09:39:57Z

jspwiki-util/src/main/java/org/apache/wiki/util/Synchronizer.java

+     * } finally {
+     *     lock.unlock();
+     * }
+     * }


duplicated line, can be deleted?

juanpablo-santos · 2023-10-11T09:41:55Z

jspwiki-util/src/main/java/org/apache/wiki/util/Synchronizer.java

+    public static <E extends Exception> void synchronize(final ReentrantLock lock, final ThrowingRunnable<E> throwingRunnable) {
+        lock.lock();
+        try {
+            throwingRunnable.run();


didn't see this initially, good catch!!

jspwiki-event/pom.xml

juanpablo-santos · 2023-10-11T09:50:38Z

pom.xml

+
+      <dependency>
+        <groupId>org.apache.jspwiki</groupId>
+        <artifactId>jspwiki-util</artifactId>


this shouldn't be needed here, jspwiki modules should be referenced on other modules using groupId:artifactId:version where groupId is always ${project.groupId} and version always is ${project.version} (see this for an example)

arturobernalg · 2023-10-11T19:54:52Z

HI @juanpablo-santos
I fix all the comment. Please do another pass.
TY

juanpablo-santos

apologies on the delay reviewing this PR, there's lots of code and very little time :-S

juanpablo-santos · 2023-12-02T21:27:39Z

jspwiki-event/src/main/java/org/apache/wiki/event/WikiEventManager.java

-        synchronized( m_preloadCache ) {
-            m_preloadCache.clear();
-        }
+        Synchronizer.synchronize(delegatesLockLock, m_delegates::clear);


m_delegates could be converted to a ConcurrentHashMap, avoiding the need of using Synchronizer.synchronize. Also m_preloadCache is a Vector, hence thread-safe, so there's no need of syncing.. don't know why it was synchronized in first place, though.

juanpablo-santos · 2023-12-02T21:28:41Z

jspwiki-event/src/main/java/org/apache/wiki/event/WikiEventManager.java

@@ -314,35 +337,35 @@ private Map< Object, WikiEventDelegate > getDelegates() {
     * @param client   the client Object, or alternately a Class reference
     * @return the WikiEventDelegate.
     */
-    private WikiEventDelegate getDelegateFor( final Object client ) {
-        synchronized( m_delegates ) {


again, converting to ConcurrentHashMap would avoid all the syncing..

juanpablo-santos · 2023-12-02T21:32:06Z

jspwiki-event/src/main/java/org/apache/wiki/event/WikiEventManager.java

@@ -406,15 +429,15 @@ public Set< WikiEventListener > getWikiEventListeners() {
         * @return true if the listener was added (i.e., it was not already in the list and was added)
         */
        public boolean addWikiEventListener( final WikiEventListener listener ) {
-            synchronized( m_listenerList ) {
+            return Synchronizer.synchronize(wikiEventListenerLock, () -> {


shouldn't m_listener reuse the same Lock through the entire class? Different locks on different operations would imply they could execute operations on m_listenerList at the same time

juanpablo-santos · 2023-12-02T21:36:14Z

jspwiki-main/src/main/java/org/apache/wiki/WatchDog.java

@@ -136,26 +154,26 @@ private static void scrub() {
     *  Can be used to enable the WatchDog.  Will cause a new Thread to be created, if none was existing previously.
     */
    public void enable() {
-        synchronized( WatchDog.class ) {
+        Synchronizer.synchronize(enableLock, () -> {


same as before, same synchronized object should reuse the same Lock throughout the class.

juanpablo-santos · 2023-12-02T21:36:39Z

jspwiki-main/src/main/java/org/apache/wiki/WatchDog.java

@@ -186,10 +204,10 @@ public void enterState( final String state ) {
     */
    public void enterState( final String state, final int expectedCompletionTime ) {
        LOG.debug(  "{}: Entering state {}, expected completion in {} s", m_watchable.getName(), state, expectedCompletionTime );
-        synchronized( m_stateStack ) {
+        Synchronizer.synchronize(enterStateLock, () -> {


same as before..

juanpablo-santos · 2023-12-02T21:40:00Z

jspwiki-main/src/main/java/org/apache/wiki/auth/authorize/DefaultGroupManager.java

@@ -152,13 +180,13 @@ public void initialize( final Engine engine, final Properties props ) throws Wik

        // Load all groups from the database into the cache
        final Group[] groups = m_groupDatabase.groups();
-        synchronized( m_groups ) {


converting m_groups to ConcurrentHashMap would avoid the use of Synchronizer

juanpablo-santos · 2023-12-02T22:17:15Z

jspwiki-main/src/main/java/org/apache/wiki/auth/authorize/DefaultGroupManager.java

@@ -367,14 +395,18 @@ protected String[] extractMembers( final String memberLine ) {

    /** {@inheritDoc} */
    @Override
-    public synchronized void addWikiEventListener( final WikiEventListener listener ) {
-        WikiEventManager.addWikiEventListener( this, listener );
+    public void addWikiEventListener( final WikiEventListener listener ) {


should use the same lock as removeWikiEventListener

juanpablo-santos · 2023-12-02T22:19:04Z

jspwiki-main/src/main/java/org/apache/wiki/plugin/PageViewPlugin.java

+     *
+     * @see java.util.concurrent.locks.ReentrantLock
+     */
+    private final ReentrantLock lock;


better private final ReentrantLock lock = new ReentrantLock(); to avoid to declare the constructor?

juanpablo-santos · 2023-12-02T22:22:03Z

jspwiki-main/src/main/java/org/apache/wiki/search/LuceneSearchProvider.java

+     *
+     * @see java.util.concurrent.locks.ReentrantLock
+     */
+    private final ReentrantLock lock;


as noted before, perhaps better to just private final ReentrantLock lock = new ReentrantLock(); to avoid declaring the constructor? Also, seems that more than one lock is needed

juanpablo-santos · 2023-12-02T22:23:04Z

jspwiki-main/src/main/java/org/apache/wiki/search/LuceneSearchProvider.java

@@ -389,9 +407,9 @@ protected Document luceneIndexPage( final Page page, final String text, final In
            field = new Field( LUCENE_PAGE_KEYWORDS, page.getAttribute( "keywords" ).toString(), TextField.TYPE_STORED );
            doc.add( field );
        }
-        synchronized( writer ) {
+        Synchronizer.synchronize(lock, () -> {


additional lock here?

Refactored synchronized blocks/methods containing blocking operations to prevent potential deadlocks introduced by the upcoming Virtual Threads feature in JDK 21.

+ some format edits

…which is not happening here

juanpablo-santos · 2024-04-06T12:17:52Z

Hi @arturobernalg !

went ahead and pushed remaining requested changes, however I've got tests errors on XMLUserDatabaseTest. They aren't happening on the master branch, and were happening before applying my commits. Don't know if they're flaky tests which fail due to previous tests setting some unwanted state or if a bug was introduced with the refactor, would you mind taking a look at it?

thx + best regards

arturobernalg · 2024-04-07T16:47:00Z

Hi @arturobernalg !

went ahead and pushed remaining requested changes, however I've got tests errors on XMLUserDatabaseTest. They aren't happening on the master branch, and were happening before applying my commits. Don't know if they're flaky tests which fail due to previous tests setting some unwanted state or if a bug was introduced with the refactor, would you mind taking a look at it?

thx + best regards

Hi @juanpablo-santos

I'll take a look.

TY

juanpablo-santos · 2024-05-17T13:20:33Z

hey @arturobernalg just found out about https://projectlombok.org/features/Locked on latest Lombok; same approach as your Synchronizer class, which seems definitely the way to go :-)

arturobernalg · 2024-05-24T17:14:01Z

hey @arturobernalg just found out about https://projectlombok.org/features/Locked on latest Lombok; same approach as your Synchronizer class, which seems definitely the way to go :-)

Hey @juanpablo-santos thanks for sharing! I'm continuing to check the issues.

arturobernalg requested a review from juanpablo-santos September 11, 2023 20:58

arturobernalg force-pushed the JSPWIKI-1178 branch from ace15eb to 0af7dcf Compare September 11, 2023 21:00

arturobernalg force-pushed the JSPWIKI-1178 branch from 0af7dcf to 5b7704b Compare October 9, 2023 16:55

juanpablo-santos requested changes Oct 11, 2023

View reviewed changes

arturobernalg force-pushed the JSPWIKI-1178 branch 2 times, most recently from 2a81421 to 62b140b Compare October 11, 2023 19:54

arturobernalg requested a review from juanpablo-santos October 11, 2023 19:54

arturobernalg force-pushed the JSPWIKI-1178 branch 2 times, most recently from c1edd8a to b9b3a38 Compare October 19, 2023 19:50

arturobernalg force-pushed the JSPWIKI-1178 branch from b9b3a38 to 659b8a5 Compare November 26, 2023 19:09

juanpablo-santos requested changes Dec 2, 2023

View reviewed changes

arturobernalg and others added 2 commits December 6, 2023 09:32

[JSPWIKI-1178] Address potential deadlock with JDK 21 Virtual Threads.

e50fdc7

Refactored synchronized blocks/methods containing blocking operations to prevent potential deadlocks introduced by the upcoming Virtual Threads feature in JDK 21.

Use custom Synchronizer class to manage ReentrantLock synchronization

3a83d94

arturobernalg force-pushed the JSPWIKI-1178 branch from 659b8a5 to 3a83d94 Compare December 6, 2023 08:34

juanpablo-santos added 8 commits April 6, 2024 14:06

Move ReentrantLock instantiation to class level variables

1fa8bd7

+ some format edits

Requested changes on PR and comments on ML

fb83d61

Requested changes

5a34ccd

Synchronize only on delegated getInstance method

88a75eb

requested changes

8e128cd

Move ReentrantLock instantiation to class level variable

540ccd2

IndexWriter can cause sync issues only if its shared across threads, …

0fd016c

…which is not happening here

revert code change on PR to original while condition

15898f0

juanpablo-santos added the reviewed/changes-requested label Sep 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JSPWIKI-1178] Address potential deadlock with JDK 21 Virtual Threads. #307

[JSPWIKI-1178] Address potential deadlock with JDK 21 Virtual Threads. #307

arturobernalg commented Sep 11, 2023

juanpablo-santos commented Oct 2, 2023

arturobernalg commented Oct 3, 2023

juanpablo-santos commented Oct 9, 2023 •

edited

Loading

arturobernalg commented Oct 9, 2023 •

edited

Loading

arturobernalg commented Oct 10, 2023 •

edited

Loading

juanpablo-santos left a comment

juanpablo-santos Oct 11, 2023

juanpablo-santos Oct 11, 2023

juanpablo-santos Oct 11, 2023

juanpablo-santos Oct 11, 2023

juanpablo-santos Oct 11, 2023

juanpablo-santos Oct 11, 2023

juanpablo-santos Oct 11, 2023

juanpablo-santos Oct 11, 2023

juanpablo-santos Oct 11, 2023

arturobernalg commented Oct 11, 2023

juanpablo-santos left a comment

juanpablo-santos Dec 2, 2023

juanpablo-santos Dec 2, 2023

juanpablo-santos Dec 2, 2023

juanpablo-santos Dec 2, 2023

juanpablo-santos Dec 2, 2023

juanpablo-santos Dec 2, 2023

juanpablo-santos Dec 2, 2023

juanpablo-santos Dec 2, 2023

juanpablo-santos Dec 2, 2023

juanpablo-santos Dec 2, 2023

juanpablo-santos commented Apr 6, 2024

arturobernalg commented Apr 7, 2024

juanpablo-santos commented May 17, 2024

arturobernalg commented May 24, 2024

[JSPWIKI-1178] Address potential deadlock with JDK 21 Virtual Threads. #307

Are you sure you want to change the base?

[JSPWIKI-1178] Address potential deadlock with JDK 21 Virtual Threads. #307

Conversation

arturobernalg commented Sep 11, 2023

juanpablo-santos commented Oct 2, 2023

arturobernalg commented Oct 3, 2023

juanpablo-santos commented Oct 9, 2023 • edited Loading

arturobernalg commented Oct 9, 2023 • edited Loading

arturobernalg commented Oct 10, 2023 • edited Loading

juanpablo-santos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturobernalg commented Oct 11, 2023

juanpablo-santos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanpablo-santos commented Apr 6, 2024

arturobernalg commented Apr 7, 2024

juanpablo-santos commented May 17, 2024

arturobernalg commented May 24, 2024

juanpablo-santos commented Oct 9, 2023 •

edited

Loading

arturobernalg commented Oct 9, 2023 •

edited

Loading

arturobernalg commented Oct 10, 2023 •

edited

Loading