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

[arturobernalg]

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

[juanpablo-santos]

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]

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]

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]

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]

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.

[arturobernalg]

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

[juanpablo-santos]

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]

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