ApplicationReadyEvent fires multiple time while using SpringApplicationBuilder

[LeckerMaedschen]

Using ApplicationListener<ApplicatioReadyEvent> with the usage of SpringApplicationBuilder fires multiple notification to that listener (for every child). Please have a look to that small demonstration: demo.zip

Issue created after stackoverflow question comment: here

[philwebb]

This mainly due to the mismatch between SpringApplicationBuilder and SpringApplication. Currently only the builder is aware of parent/child relationships.

I agree that it's a little odd to get the ApplicatioReadyEvent even more than once but I'm also worried that people might be relying on it.

One option you might have is to check for getParent() == null in your listener:

    public void onApplicationEvent(ApplicationReadyEvent e) {
        if (e.getApplicationContext().getParent == null) {
            System.out.println("******************************");
            System.out.println("Post-process begins.");
            System.out.println("******************************");      
        }
    }

[Riggs333]

@LeckerMaedschen why not register the listener only on one of the children? Like this:

public class Application {

    public static void main(String[] args) {
        SpringApplicationBuilder parentBuilder
                = new SpringApplicationBuilder(Application.class);
//        parentBuilder.listeners(new AfterStart());

        parentBuilder.child(Config1.class)
                .properties("server.port:8443")
                .listeners(new AfterStart())  // register listener only on this child
                .run(args);
        parentBuilder.child(Config2.class)
                .properties("server.port:9443")
                .run(args);

    }
}

[snicoll]

Regardless I think we should fix it somehow. If we agree the scope of the application is independent of the structure of the application context, then we should fire it only once and at the appropriate time. Not sure however that everybody share the same concept of "application" (perhaps the builder is used to start several apps in isolation in child context?).

[philwebb]

We've discussed in the past if there shouldn't be a better way for SpringApplication to create parent/child contexts. If that was there it would remove the need for SpringApplicationBuilder to call multiple SpringApplication instances.

[dsyer]

There was a related, but not identical issue in Spring Cloud. We wanted more flexibility in the hierarchy building, so we could identify the parent context when the child is being created. The solution for now is a reflective workaround: https://github.com/spring-cloud/spring-cloud-commons/blob/master/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/BootstrapApplicationListener.java#L104 (see findBootstrapContext() method). Maybe these two issues together show some angles of attack for designing a new API.

[wilkinsona]

It feels like we should either tackle this in 2.0 or close it as a known limitation. Thoughts?

[philwebb]

I vote known issue.

[jkubrynski]

As discussed with @dsyer on gitter I think it would be useful to be able to check what kind of context triggered the event -> application context, management context, cloud-stream or feign. Maybe a qualifier or something similar to determine a context without hacks? Other solution could be an event that the main application context has started. However, there are already a lot of events related to this, so I'm not sure it it's a best idea to add another one

[wilkinsona]

The management context won't fire an ApplicationReadyEvent. If you'd like a Cloud Stream- or Feign-created context to provide some sort of qualifier to determine the context then that would have to be something done by Cloud Stream or Feign.

[jkubrynski]

@wilkinsona I think it'd be easier to add some root context qualifier in spring-boot instead of adding qualifiers in all technologies that start the context. Otherwise, you'll need to take care about any module, that starts its context.

[dsyer]

Let's ask @garyrussell what he thinks. It might actually be a bug in Spring Cloud Stream, since I know other child contexts do not suffer the same way exactly (cf Andy's comment about the management context, and also Feign). Perhaps Stream child contexts are too "heavy"? Or perhaps there's a reason they need to include all the listeners from the parent.

[garyrussell]

If someone can provide an example that exhibits the problem with Spring Cloud Stream, I'd be happy to take a look.

I don't see a problem with Stream if the listener is a @Bean - https://gitter.im/spring-cloud/spring-cloud-stream?at=59db7585bbbf9f1a383cb9de

@dsyer said:

I guess if it's a @bean it works
If you register the listener in the SpringApplication or via spring.factories I think it's different

I just need a pointer to reproduce.

[dsyer]

@SpringBootApplication
@EnableBinding(Processor.class)
public class StreamListenerApplication {

    public static void main(String[] args) {
        new SpringApplicationBuilder(StreamListenerApplication.class)
                .listeners(new Listener()).run(args);
    }
}

class Listener implements ApplicationListener<ApplicationReadyEvent> {

    @Override
    public void onApplicationEvent(ApplicationReadyEvent event) {
        System.err.println("Ready: " + event.getApplicationContext().hashCode());
    }

}

Prints "Ready" twice. And there's no way to know from the listener that you are in the "main" context or not.

The Bootstrap context is also there (created by Spring Cloud Context) and it uses SpringApplicationBuilder, but explicitly doesn't share listeners. Also if you set management.port=8081 it still prints "Ready" twice - the actuator creates a child context that doesn't have all the listeners from the main application. Ditto Feign.

[garyrussell]

Yikes!

IMHO, such listeners should be registered as beans in the main context and initialized like any other bean (@Autowired, ...Aware injection etc).

[wilkinsona]

IMHO, such listeners should be registered as beans in the main context and initialized like any other bean (@Autowired, ...Aware injection etc).

Depending on the events that you want to consume, you can't always do that. For example, ApplicationEnvironmentPreparedEvent is published before the application context has been created.

[garyrussell]

Right, but you could still do it before adding to the context.

@Override
public void contextLoaded(ConfigurableApplicationContext context) {
	for (ApplicationListener<?> listener : this.application.getListeners()) {
		if (listener instanceof ApplicationContextAware) {
			((ApplicationContextAware) listener).setApplicationContext(context);
		}
		context.addApplicationListener(listener);
	}
	this.initialMulticaster.multicastEvent(
			new ApplicationPreparedEvent(this.application, this.args, context));
}

I would suggest running the Annotation BPP against the listener and context.initializeBean() to inject the ...Aware impls before addApplicationListener().

[wilkinsona]

Sorry, I don't understand how that would help. The context is already available from the ApplicationPreparedEvent so I don't see any benefit to calling setApplicationContext on the listeners prior to multicasting the event to them.

I think the problem here is really that Spring Cloud Stream creates a whole new child application when just a child context may well be sufficient. This is what the Actuator does when creating its child context when a separate management HTTP port is required. If SCS just used a child context, none of the Application…Events would be multicast. Alternatively, it could continue to use SpringApplication but not copy the listeners from the parent as @dsyer suggests above.

[dsyer]

I'm not sure I'm following all of this, but having an ApplicationContext injected in to the listener is not the same as having access to it in the event (they might be different instances, after all, which is somewhat the point of the discussion). I thought we used to do *Aware injection into Boot listeners. Maybe I misremembered, or maybe we took it out for some reason. Sounds like a reasonable proposal to me.

[wilkinsona]

It sounds dangerous to me, particularly if the listeners are being copied from a parent SpringApplication to a child. You'd then have setApplicationContext called multiple times on the same listener. A listener that was actually still registered in the parent application would have its application context set twice so it ended up looking like it was registered in the child.

[dsyer]

I'm not sure, but I don't think any actual listener instances are copied. It's just that any registered via spring.factories end up being in both contexts (different instances).

[wilkinsona]

For those coming from spring.factories, I see no benefit in setting the application context. What will it tell you that you can't already learn by looking at the context in the event?

[dsyer]

That's the whole point I think. You get events from all the contexts in the hierarchy, but you can't tell from a listener if their source is from "your" application or not.

[wilkinsona]

Aaah, I get it now. Thanks for bearing with me.

So for the specific problem raised by this issue, you can use register your ApplicationListener<ApplicationReadyEvent> as a bean rather than directly with SpringApplication, inject the context (via autowiring or application context aware) and the filter out any events that you don't care about.

This solution won't work for events that are multicast before any application listener beans are instantiated. Those are ApplicationStartingEvent, ApplicationEnvironmentPreparedEvant, and ApplicationPreparedEvent. Of these, only ApplicationPreparedEvent has a context available and the code that @garyrussell pasted above is already in place, i.e. we already perform ApplicationContextAware injection for ApplicationListener instances registered with SpringApplication. I am not in favour of expanding that to running any bean post-processors against the listeners as they are not beans. We could, however, consider also honouring EnvironmentAware prior to multicasting the ApplicationEnvironmentPreparedEvent.

With or without the EnvironmentAware change, this issue could then be resolved by documenting the behaviour and the use of ApplicationContextAware to inject the listener's context and compare it with the event's context to see if it's of interest.

[dsyer]

I was able to simplify my listeners in the benchmarks quite a lot, now that I know about the ApplicationContextAware trick (in fact I did already know, but had convinced myself I was wrong apparently). So documentation is probably all we need on that (if anything). Adding support for EnvironmentAware doesn't really seem necessary (you have everything you need from the 2 application contexts).

[garyrussell]

i.e. we already perform ApplicationContextAware injection for ApplicationListener

Doh - I didn't notice that code. I guess I am ok with not doing auto wiring - agreed that doco is probably enough at this point.

That said, I am sure I would only use this feature when interested with those "early" events and use a @Bean for a "proper" listener.