Move the creation of bootstrap beans into create function #327

ryanjbaxter · 2024-01-05T01:14:42Z

The change I made in #317 which registered a new ConfigSeverInstanceProvider.Function only partially worked. All the beans that were registered in the initialize method really didn't do anything anymore. They all check if the right properties are enabled but that always resulted in false because the properties are not set in the environment when Boot initially load configuration.

Further more the new ConfigServerInstanceProvider.Function did not check if existing beans were already registered. I attempted to fix that in #325 but that fix did not work as I thought it did.

This fix should truly fix #324

ryanjbaxter · 2024-01-05T01:16:19Z

...pringframework/cloud/zookeeper/discovery/configclient/ZookeeperConfigServerBootstrapper.java

@@ -67,54 +71,6 @@ public void initialize(BootstrapRegistry registry) {
 				ClassUtils.isPresent("org.springframework.cloud.bootstrap.marker.Marker", null)) {
 			return;
 		}
-		// create curator
-		CuratorFactory.registerCurator(registry, null, true, bootstrapContext -> isEnabled(bootstrapContext.get(Binder.class)));


The isEnabled check in the code below always resulted in false because Boot had not fully loaded configuration properties at this point. None of these registered beans actually returned anything.

I may be missing something here, but didn't the isEnabled calls actually get executed later, when the beans were instantiated for real, as opposed to when they were registered into the bootstrap context?

(Ref.: https://github.com/spring-cloud/spring-cloud-zookeeper/blob/4.0.x/spring-cloud-zookeeper-core/src/main/java/org/springframework/cloud/zookeeper/CuratorFactory.java#L92-L127)

The isEnabled call would be executed as soon as the bean was needed. In most cases that would have happened within ZookeeperFunction.apply. However the Binder from the BootstrapContext in the registerIfAbsent function is not the same as the one that is used in theZookeeper.apply method. In the apply method we are getting the Binder while Boot is in the process of gathering all property sources.

See https://github.com/spring-cloud/spring-cloud-config/blob/main/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServerConfigDataLocationResolver.java#L240
and
https://github.com/spring-cloud/spring-cloud-config/blob/main/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServerConfigDataLocationResolver.java#L231

This Binder contains all resolved property sources at this point that may have not been applied to the BootstrapContext yet. It is very confusing I know but this is the crux as to why I changed this code in the first place.

Yeah, I've been looking at what's going on here and I think I'm starting to understand this problem with the Binder. It is indeed very complex and confusing, unfortunately. :(

I was wondering, why do we have a "less knowledgeable" Binder in the BootstrapContext at that point in time, when a "more knowledgeable" already exists? The Binder in the bootstrap context seems to get re-registered a number of times by the calls to ConfigDataEnvironment.registerBootstrapBinder, could it not get updated once more before all the ZooKeeper & Curator beans need to be instantiated? Or is this resolverContext.getBinder() some specialized one-off instance that would not be usable as the generic bootstrap context Binder? I'm still trying to understand what the intended lifecycle of all the things involved are. (If you have any relevant documentation you can share on these low-level implementation details, that would be much appreciated. 🙂) I noticed this part of the code is not Spring Cloud anymore, but Spring Boot. Is it possible to make some changes there too, if that would lead to a more elegant solution here, or is that out of the question?

The other idea I had was that we could perhaps register another bean into the BootstrapContext, just before we register ConfigServerInstanceMonitor, something from where one could retrieve this "more knowledgeable" Binder. This could maybe be the ConfigDataLocationResolverContext resolverContext itself, maybe some new class' instance that is specific to Spring Cloud Config and this use-case. This would allow for the original code to be modified only slightly, e.g.:

registry.registerIfAbsent(ConfigServerInstanceProvider.Function.class, context -> { if (!isEnabled(context.get(ConfigDataLocationResolverContext.class).getBinder())) { return (id) -> Collections.emptyList(); } return context.get(ZookeeperDiscoveryClient.class)::getInstances; });

This way all the other beans could again be registered into the bootstrap context, like before, and much of the confusion would also be avoided (at least the out-of-the ordinary passing around of the "more knowledgeable" Binder). Unfortunately this is just an idea at this point, I didn't have time to (dis)prove it yet. Maybe you've considered this already and can save me some time if you know it won't work. 🙂

Is it possible to make some changes there too, if that would lead to a more elegant solution here, or is that out of the question?

Possibly, but you would need to open an issue in Spring Boot and discuss it with them

The other idea I had was that we could perhaps register another bean into the BootstrapContext, just before we register ConfigServerInstanceMonitor, something from where one could retrieve this "more knowledgeable" Binder. This could maybe be the ConfigDataLocationResolverContext resolverContext itself, maybe some new class' instance that is specific to Spring Cloud Config and this use-case.

I can try this an see if it would work, but it might take a few days

ryanjbaxter · 2024-01-05T01:17:24Z

...pringframework/cloud/zookeeper/discovery/configclient/ZookeeperConfigServerBootstrapper.java

@@ -123,11 +79,19 @@ public void initialize(BootstrapRegistry registry) {

 		// promote beans to context
 		registry.addCloseListener(event -> {
-			ZookeeperDiscoveryClient discoveryClient = event.getBootstrapContext().get(ZookeeperDiscoveryClient.class);


Since the beans aren't registered in the bootstrap context we need to get them from ZookeeperFunction to promote them

Is it an intentional decision not to add these beans into the bootstrap context anymore, or was it done out of necessity only? I.e. was this change done because this is semantically "the right thing" to do, and the old behavior of registering them into the context is considered incorrect, or because there seemed to be no better way to fix the bug?

It was done out of necessity. As I described above the Binder does not contain the property sources to correctly resolve the isEnabled call when Boot is trying to load all its configuration, so this code no longer worked.

ryanjbaxter · 2024-01-05T01:18:06Z

...pringframework/cloud/zookeeper/discovery/configclient/ZookeeperConfigServerBootstrapper.java

 			if (discoveryClient != null) {
 				event.getApplicationContext().getBeanFactory().registerSingleton("zookeeperServiceDiscovery",
 						discoveryClient);
 			}
+
+			CuratorFramework curatorFramework = zookeeperFunction.getCuratorFramework();


This used to be done by CuratorFactory.registerCurator but we don't call that anymore so we need to promote it here

ryanjbaxter · 2024-01-05T01:19:46Z

@pantherdd would appreciate your eyes on this as well. My last fix didn't end up working, I think this ultimately solves #324

pantherdd · 2024-01-08T10:33:29Z

@ryanjbaxter Sure, I'll try to find some time for it this week.

spencergibb

Assuming a 👍🏻 from @pantherdd

pantherdd · 2024-01-09T10:35:31Z

...pringframework/cloud/zookeeper/discovery/configclient/ZookeeperConfigServerBootstrapper.java

@@ -67,54 +71,6 @@ public void initialize(BootstrapRegistry registry) {
 				ClassUtils.isPresent("org.springframework.cloud.bootstrap.marker.Marker", null)) {
 			return;
 		}
-		// create curator
-		CuratorFactory.registerCurator(registry, null, true, bootstrapContext -> isEnabled(bootstrapContext.get(Binder.class)));


I may be missing something here, but didn't the isEnabled calls actually get executed later, when the beans were instantiated for real, as opposed to when they were registered into the bootstrap context?

(Ref.: https://github.com/spring-cloud/spring-cloud-zookeeper/blob/4.0.x/spring-cloud-zookeeper-core/src/main/java/org/springframework/cloud/zookeeper/CuratorFactory.java#L92-L127)

pantherdd · 2024-01-09T10:51:32Z

...pringframework/cloud/zookeeper/discovery/configclient/ZookeeperConfigServerBootstrapper.java

@@ -123,11 +79,19 @@ public void initialize(BootstrapRegistry registry) {

 		// promote beans to context
 		registry.addCloseListener(event -> {
-			ZookeeperDiscoveryClient discoveryClient = event.getBootstrapContext().get(ZookeeperDiscoveryClient.class);


Is it an intentional decision not to add these beans into the bootstrap context anymore, or was it done out of necessity only? I.e. was this change done because this is semantically "the right thing" to do, and the old behavior of registering them into the context is considered incorrect, or because there seemed to be no better way to fix the bug?

pantherdd · 2024-01-09T10:53:39Z

...pringframework/cloud/zookeeper/discovery/configclient/ZookeeperConfigServerBootstrapper.java

@@ -123,11 +79,19 @@ public void initialize(BootstrapRegistry registry) {

 		// promote beans to context
 		registry.addCloseListener(event -> {
-			ZookeeperDiscoveryClient discoveryClient = event.getBootstrapContext().get(ZookeeperDiscoveryClient.class);
+			ZookeeperFunction zookeeperFunction = ((ZookeeperFunction) event.getBootstrapContext().get(ConfigServerInstanceProvider.Function.class));


This cast will throw if someone tries to register their own ConfigServerInstanceProvider.Function, for example as a workaround. And ZookeeperFunction is non-public as well as final, so there's no way to subclass it to work around the ClassCastException.

I think we can make it public and non-final then. That would resolve this, agree?

It would be much better if we could completely avoid this "mini-context" class somehow (hence my other ideas in the other comments), but yes, I think doing so would help if ZookeeperFunction has to stay.

pantherdd · 2024-01-16T12:12:31Z

...pringframework/cloud/zookeeper/discovery/configclient/ZookeeperConfigServerBootstrapper.java

-				log.warn("Error fetching config server instance from Zookeeper", e);
-				return Collections.emptyList();
+			if (this.discoveryClient == null) {
+				ZookeeperProperties properties = context.getOrElseSupply(ZookeeperProperties.class, () -> binder.bind(ZookeeperProperties.PREFIX, Bindable.of(ZookeeperProperties.class))


I tried applying this ZookeeperFunction fix in my test project, but it didn't quite help in the end. The problem was that my customization code, that registers my ZookeeperProperties instance, had the same isEnabled condition as Spring itself, because I wanted to have the same activation behavior. But this creates the same problem with the Binder; I would need access to the "more knowledgeable" one when I register my custom beans.

I think you can safely assume at the point where the beans are being requested from the bootstrap context that we have already done the isEnabled check (or we wouldn't be requesting them) so you shouldn't have to repeat that check for your custom beans

Hmm, I guess that sounds reasonable. However I was simply copying that approach from ZookeeperConfigServerBootstrapper.initialize and CuratorFactory.registerCurator – should the isEnabled guard be removed from all of the bootstrap context bean registrations except for a few (e.g. CuratorFramework)? I'm trying to be as consistent with the framework's internals as possible.

pantherdd · 2024-01-24T07:49:05Z

For the record, this was superseded by #328 as the final solution to #324.

Move the creation of boostrap beans into create function

4195124

ryanjbaxter requested a review from spencergibb January 5, 2024 01:14

ryanjbaxter added the bug label Jan 5, 2024

ryanjbaxter added this to the 4.0.5 milestone Jan 5, 2024

ryanjbaxter commented Jan 5, 2024

View reviewed changes

spencergibb approved these changes Jan 11, 2024

View reviewed changes

pantherdd reviewed Jan 12, 2024

View reviewed changes

pantherdd reviewed Jan 16, 2024

View reviewed changes

ryanjbaxter mentioned this pull request Jan 18, 2024

Revert changes in config server bootstrapper and use new PropertyResolver class #328

Merged

pantherdd mentioned this pull request Jan 24, 2024

Regression in 3.1.4: customizations to the CuratorFramework instance are now silently ignored #324

Closed

ryanjbaxter closed this Jan 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move the creation of bootstrap beans into create function #327

Move the creation of bootstrap beans into create function #327

ryanjbaxter commented Jan 5, 2024

ryanjbaxter Jan 5, 2024

pantherdd Jan 9, 2024

ryanjbaxter Jan 12, 2024

pantherdd Jan 16, 2024 •

edited

Loading

ryanjbaxter Jan 16, 2024

ryanjbaxter Jan 5, 2024

pantherdd Jan 9, 2024

ryanjbaxter Jan 12, 2024

ryanjbaxter Jan 5, 2024

ryanjbaxter commented Jan 5, 2024

pantherdd commented Jan 8, 2024

spencergibb left a comment

pantherdd Jan 9, 2024

pantherdd Jan 9, 2024

pantherdd Jan 9, 2024

ryanjbaxter Jan 12, 2024

pantherdd Jan 16, 2024

pantherdd Jan 16, 2024

ryanjbaxter Jan 16, 2024

pantherdd Jan 16, 2024 •

edited

Loading

pantherdd commented Jan 24, 2024

Move the creation of bootstrap beans into create function #327

Move the creation of bootstrap beans into create function #327

Conversation

ryanjbaxter commented Jan 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pantherdd Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanjbaxter commented Jan 5, 2024

pantherdd commented Jan 8, 2024

spencergibb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pantherdd Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

pantherdd commented Jan 24, 2024

pantherdd Jan 16, 2024 •

edited

Loading

pantherdd Jan 16, 2024 •

edited

Loading