Use lock for instance status update. #1566
Conversation
eureka-client/src/main/java/com/netflix/discovery/DiscoveryClient.java
Outdated
Show resolved
Hide resolved
OlgaMaciaszek
force-pushed
the
2.x-use-lock-for-instance-status-update
branch
from
f7d808d to
ca2d613
Compare
| logger.warn("Exception from healthcheckHandler.getStatus, setting status to DOWN", e); | ||
| status = InstanceStatus.DOWN; | ||
| } | ||
| if (instanceStatusUpdateLock.tryLock()) { |
There was a problem hiding this comment.
I'm not sure if we need the fix here? Since both threads will call healthcheck to find latest status, unless healthcheck itself is flipping, the status would be the same? Maybe there is a race in update sequence, but the value would be consistent?
There was a problem hiding this comment.
Hi @howardyuan, thanks for the comment. You're right. I've taken another look at it and I now think the approach to take could be to have a compareAndSet logic while changing InstanceInfo.InstanceStatus within DiscoveryClient#refreshInstanceInfo to prevent a situation where the status has been changed by the other thread between getting original status from the field and setting its transformed value there. Could be done by changing that field into an actual AtomicReference or, to remain consistent with the current class implementation and avoid having to change/ add too many methods, keeping it as a volatile field and adding a synchronized method in that class that would first compare the expected field value with the actual one and calling that new method within DiscoveryClient#refreshInstanceInfo. Let me know what you think.
There was a problem hiding this comment.
I think your suggestions of comparing before setting should be safe.
There was a problem hiding this comment.
Will work on it soon.
This is related to spring-cloud/spring-cloud-netflix#4094 as an attempted fix.