Implement semaphore synchronization to prevent concurrent modificatio…issue#4094 #4301
Conversation
…ns of InstanceStatus Implement semaphore synchronization to prevent concurrent modifications of InstanceStatus - Added a Semaphore to control concurrent access to InstanceStatus modifications. - Modified EurekaClientConfigurationRefresher to acquire the semaphore before performing deregister and register operations. - Modified DiscoveryClientConfiguration to acquire the semaphore before performing refreshInstanceInfo operations. - Ensured the semaphore is released in a finally block to handle exceptions properly. This change addresses the issue of concurrent modifications of InstanceStatus, ensuring that only one thread can modify the status at a time.
|
Fixes gh-4094. |
There was a problem hiding this comment.
Hi @bazzop, thanks for submitting the PR. The build is failing on checkstyle. Please apply the necessary changes and make sure the build passes. Please address the comments I've added. Also, please submit your changes against 4.1.x instead of main so that we can get the bugfix also in the 2023.0.x release train.
| @@ -51,52 +52,100 @@ | |||
| @ConditionalOnBlockingDiscoveryEnabled | |||
| public class EurekaDiscoveryClientConfiguration { | |||
|
|
|||
| @Bean | |||
| @ConditionalOnMissingBean | |||
| public EurekaDiscoveryClient discoveryClient(EurekaClient client, EurekaClientConfig clientConfig) { | |||
There was a problem hiding this comment.
It seems you've changed formatting - as a result it shows as if you've modified all these lines of code. Please revert to our formatting (possibly a spaces/ tabs issue) so that it's clearly visible in the diff what code has been changed by you.
| @@ -19,7 +19,6 @@ | |||
| import com.netflix.appinfo.HealthCheckHandler; | |||
| import com.netflix.discovery.EurekaClient; | |||
| import com.netflix.discovery.EurekaClientConfig; | |||
There was a problem hiding this comment.
Please update the date in the license comment to 2013-2024.
| @@ -35,6 +34,8 @@ | |||
| import org.springframework.context.annotation.Bean; | |||
| import org.springframework.context.annotation.Configuration; | |||
|
|
|||
| import java.util.concurrent.Semaphore; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Please add your full name with @author tag to the javadoc of the class.
|
|
||
| void refreshInstanceInfo() { | ||
| try { | ||
| semaphore.acquire(); |
There was a problem hiding this comment.
Why use binary semaphore rather than a ReentrantLock?
| @Autowired | ||
| private InstanceInfo instanceInfo; | ||
|
|
||
| void refreshInstanceInfo() { |
There was a problem hiding this comment.
It seems this method is not ever used - please verify and finish up your changes, and then request another review.
|
Please mark your PR as "draft" until it's ready for review. |
|
Since the comments were not address, I'm going to work on the issue. |
…ns of InstanceStatus
Implement semaphore synchronization to prevent concurrent modifications of InstanceStatus
This change addresses the issue of concurrent modifications of InstanceStatus, ensuring that only one thread can modify the status at a time.