search

spring-cloud / spring-cloud-netflix

Integration with Netflix OSS components
http://cloud.spring.io/spring-cloud-netflix/
Apache License 2.0
4.88k stars 2.44k forks source link

Race condition with registration events in Eureka server #2659

Open ghost opened 6 years ago

ghost commented 6 years ago

I believe I have found a race condition with the spring-cloud-netflix-eureka-server (Edgware) module concerning registration of instances. When an instance on the Eureka server registers, an EurekaInstanceRegisteredEvent is emitted by InstanceRegistry. Unfortunately, this event is emitted before the actual registration has taken place in the InstanceRegistry itself. Typically, one would handle this event and rely on the fact that the registration process has been completed to be able to query the registry for the new state. Especially since the class is called EurekaInstanceRegister_ED_Event.

If you have a look at the source ode of org.springframework.cloud.netflix.eureka.server.InstanceRegistry line 85+86 you can see that the event is published before super.register is called.

This behaviour affects all three events EurekaInstanceRegisteredEvent, EurekaInstanceCanceledEvent and EurekaInstanceRenewedEvent.

I am happy to provide a pull request that fixes this, unless this behaviour is on purpose (for some reason).

spencergibb commented 6 years ago

I don't think it was intentional. Is it causing a problem? Remember, eureka is eventually consistent. Even if it is published after querying eureka might not show it was registered.

tine2k commented 6 years ago

I am trying to get notified as soon as InstanceRegistry (on the same eureka vm) has updated. Correct me if I am wrong, but eventual consistency only applies in clustered mode. super.register() would immediatly (as in synchronously, same thread) update the registry right?

Can you think of another way to get notified as soon as a registration change is consistent?

spencergibb commented 6 years ago

I think it's as close as you would get.

spencergibb commented 6 years ago

Is it causing a problem?

tine2k commented 6 years ago

It causes a problem for what I am trying to do. I think emitting the event after the registration on the node has taken place is the more correct - or at least more useful since you can rely on a processed registration event in InstanceRegistry.