Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change "synchronized" to reentrant lock for virtual-threads #3652

Closed

Conversation

omercelikceng
Copy link
Contributor

This commit ensures that the method is friendly for virtual threads to avoid blocking and pinning. The lock is acquired at the beginning of the method and released in a finally block to ensure it is always released, even if an exception occurs.

@@ -267,12 +272,16 @@ public final boolean initialize() {
}
if (adminClient != null) {
try {
synchronized (this) {
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use only one try block? I think, you can move the unlocking to the finally block on the main try.

Copy link
Contributor Author

@omercelikceng omercelikceng Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sobychacko .
If I follow your suggestion, I will unnecessarily include the addOrModifyTopicsIfNeeded method within the locked block. I reviewed the addOrModifyTopicsIfNeeded method, and I believe it performs time-consuming operations. Therefore, I prefer not to place that method inside the lock block. I prefer releasing the lock as quickly as possible. If you don’t mind, I’d like to leave it as it is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I guess you can try to extract that inner try block into a separate private method - something like updateClusterId(...) and then call that in the current method. That way, we can avoid stacking one try block on another and thus look less cluttered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I'll fix it right away.

Object config = configs.get(ADD_TYPE_INFO_HEADERS);
if (config instanceof Boolean configBoolean) {
this.addTypeInfo = configBoolean;
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to remove the synchronized keyword from the method signature?

Copy link
Contributor

@sobychacko sobychacko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my in-line comments.

@omercelikceng
Copy link
Contributor Author

omercelikceng commented Nov 27, 2024

See my in-line comments.

@sobychacko I've applied the updates; could you review it again?

@sobychacko
Copy link
Contributor

@omercelikceng Thanks for the PR. Merged if via 3c97a3b.

@sobychacko sobychacko closed this Nov 27, 2024
sobychacko pushed a commit that referenced this pull request Dec 13, 2024
…pport

Fixes: #3652

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

- KafkaListenerAnnotationBeanPostProcessor
- KafkaListenerEndpointRegistrar
- KafkaAdmin
- DefaultDestinationTopicResolver
- JsonDeserializer/JsonSerializer

The change helps avoid blocking virtual threads when using Spring Kafka in Project Loom
environments while maintaining thread safety.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants