Replace synchronized blocks with ReentrantLocks for virtual thread support #102

omercelikceng · 2024-11-28T13:00:44Z

Replace synchronized methods and blocks with ReentrantLocks in a few classes in Spring Functions Catalog to improve compatibility with virtual threads. This changes the synchronization mechanism in:

CassandraConsumerConfiguration.PayloadToMatrixTransformer
InMemoryTraceRepository

The change helps avoid blocking virtual threads when using Spring Functions Catalog in Project Loom environments while maintaining thread safety.

artembilan

Let’s see if such an action is really necessary is those cases: https://mikemybytes.com/2024/02/28/curiosities-of-java-virtual-threads-pinning-with-synchronized/!
In other words: if there is really no blocking operation inside that synchronized, no reason to change it. Plus it looks like some changes are coming to Java down the road : https://openjdk.org/jeps/491

omercelikceng · 2024-11-28T19:41:40Z

Let’s see if such an action is really necessary is those cases: https://mikemybytes.com/2024/02/28/curiosities-of-java-virtual-threads-pinning-with-synchronized/! In other words: if there is really no blocking operation inside that synchronized, no reason to change it. Plus it looks like some changes are coming to Java down the road : https://openjdk.org/jeps/491

@artembilan Thank you very much for all the valuable information you provided. The links you provided contain really great information. I will read the links you sent again and again; they are really great.

However, I believe that ReentrantLock offers us more flexibility. Additionally, ReentrantLock provides features like interruptible lock acquisition, timed lock attempts, and finer control with the Condition API, which are not available with synchronized. Of course, the current code doesn't involve very complex operations, but I don't think ReentrantLock would introduce significant overhead either. Therefore, I think we could implement this improvement instead of using synchronized. However, if you disagree, I can close the pull request.

artembilan · 2024-11-28T19:43:25Z

Sure! I’ll give it a second look when back from vacation next week .

artembilan

Please, add your name to the @author list of the affected classes.

...ain/java/org/springframework/cloud/fn/consumer/cassandra/CassandraConsumerConfiguration.java

artembilan · 2024-12-02T18:51:05Z

...main/java/org/springframework/cloud/fn/consumer/websocket/trace/InMemoryTraceRepository.java

 	/**
 	 * Flag to say that the repository lists traces in reverse order.
 	 * @param reverse flag value (default true)
 	 */
 	public void setReverse(boolean reverse) {
-		synchronized (this.traces) {
+		try {
+			this.tracesLock.lock();


I don't think we need any locking in setters at all.

@artembilan I think locking is necessary here because the add method on line 86 uses the reverse and capacity values. In the add method, additions and removals are performed on the traces list using the reverse and capacity values.
add method requires locking, as any changes to reverse or capacity while the add method is being called could result in an inconsistent state. So, we need to lock both the add method and any other parts of the code that can modify the traces and reverse variables used by the add method. Therefore, I believe we should continue using locking.

What you are explaining is correct, but that's not realistic.
This class is created only once on Application context initialization.
Those setters are called at this point and only once.
Calling them at runtime is really mocking of the lifecycle of the instance of this class.

We should just agree that we are in Spring, so there are some undocumented rules for singleton beans where we just initialize them once. Plus the instance of this class is created by the configuration in this module.
You just need to drop the jar into your classpath - and everything will be auto-configured for you.

Why do we need the extra logic which may not have power in 99% of usage?

Alright, I understand what you mean. Thank you. In that case, I will remove both the lock and the synchronized.

artembilan · 2024-12-02T18:51:38Z

...main/java/org/springframework/cloud/fn/consumer/websocket/trace/InMemoryTraceRepository.java

 	}

 	public List<Trace> findAll() {
-		synchronized (this.traces) {
+		try {
+			this.tracesLock.lock();


Doesn't look like this operation needs any locking at all.

@artembilan I think the same way here as well; we shouldn’t remove the locking from this part either.

...main/java/org/springframework/cloud/fn/consumer/websocket/trace/InMemoryTraceRepository.java

artembilan · 2024-12-02T19:44:41Z

...main/java/org/springframework/cloud/fn/consumer/websocket/trace/InMemoryTraceRepository.java

 	/**
 	 * Flag to say that the repository lists traces in reverse order.
 	 * @param reverse flag value (default true)
 	 */
 	public void setReverse(boolean reverse) {
-		synchronized (this.traces) {
+		this.tracesLock.lock();


You just removed our conversion on the matter, but still left lock in the setter.
🤷

Once our conversation is finished, I was going to remove them. I'll take care of it right away.

But that conversation has not been finished.
You just didn't wait for my response.
I am the owner of the project, that the final decision is on me.

Please, don't remove any conversation: it will be helpful in the future when some one would try to figure out what was an origin of this or that code.
Like exactly someone would ask why the synchronized as removed here and lock not added.

I just thought I would make another commit after our conversation. Of course, I’ll follow your guidelines. I didn’t mean to do anything wrong, sorry if I caused any confusion.

omercelikceng · 2024-12-02T20:01:50Z

@artembilan I've applied the changes you recommended. And thank you for your guidance.

…pport

artembilan

All good, but I cannot merge it yet until I cut 5.0.x branch.
We are planning to release 5.1.0 soon enough.
Thank you!

omercelikceng · 2024-12-02T20:43:59Z

All good, but I cannot merge it yet until I cut 5.0.x branch. We are planning to release 5.1.0 soon enough. Thank you!

Thank you very much for everything. Best regards.

artembilan · 2024-12-06T22:01:42Z

@omercelikceng ,
I've changed my mind.
Since it looks like we are going to have 5.0.2 soon enough, this is a good candidate to be included there having its only internal nature.
Thank you and looking forward for more!

artembilan requested changes Nov 28, 2024

View reviewed changes

artembilan requested changes Dec 2, 2024

View reviewed changes

artembilan added this to the 5.1.0 milestone Dec 2, 2024

artembilan added the type: enhancement A general enhancement label Dec 2, 2024

artembilan requested changes Dec 2, 2024

View reviewed changes

omercelikceng added 3 commits December 2, 2024 23:10

Replace synchronized blocks with ReentrantLocks for virtual thread su…

191d099

…pport

The lock call was moved outside the try block

dfd275b

Unnecessary locking has been removed.

d4cfb37

omercelikceng force-pushed the virtualThreadSupport branch from b7cf573 to d4cfb37 Compare December 2, 2024 20:10

artembilan approved these changes Dec 2, 2024

View reviewed changes

artembilan modified the milestones: 5.1.0, 5.0.2 Dec 6, 2024

artembilan merged commit 29541a5 into spring-cloud:main Dec 6, 2024
3 checks passed

Replace synchronized blocks with ReentrantLocks for virtual thread support #102

Replace synchronized blocks with ReentrantLocks for virtual thread support #102

Uh oh!

Conversation

omercelikceng commented Nov 28, 2024

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

omercelikceng commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artembilan commented Nov 28, 2024

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omercelikceng Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omercelikceng commented Dec 2, 2024

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

omercelikceng commented Dec 2, 2024

Uh oh!

Uh oh!

artembilan commented Dec 6, 2024

Uh oh!

Uh oh!

omercelikceng commented Nov 28, 2024 •

edited

Loading

omercelikceng Dec 2, 2024 •

edited

Loading