spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.48k stars 38.09k forks source link

Introduce a RefreshableAnnotationConfigApplicationContext #28001

Closed jxblum closed 2 years ago

jxblum commented 2 years ago

This is a new feature request in the core Spring Framework to consider adding a "refreshable" AnnotationConfigApplicationContext.

To my knowledge, the only ApplicationContext implementations that allow refresh capabilities are XML configuration based ApplicationContexts and Web-based ApplicationContexts, as indicated in the Javadoc for AbstractRefreshableConfigApplicationContext.

The idea and motivation originated from a user of Spring Boot for Apache Geode (SBDG) to be able to refresh an AnnotationConfigApplicationContext in his use case. As such, I took it upon myself to create such an ApplicationContext implementation (prototype), and can be seen here.

I have written a single test case to prove it works, or rather that it is at least possible.

Certainly, much more thought and testing would be required to make this a full fledged feature of the core Spring Framework. But, I thought it was an interesting idea and perhaps a useful feature for the core Spring Framework overall.

In fact, I was bit surprised that this capability didn't already exist or that another user had not requested it, already. Perhaps someone has and I missed it, or even this has been considered before, but I did not find anything.

Anyway, happy to work with interested parties to integrate this new RefreshableAnnotationConfigApplicationContext into the core Spring Framework if the team feels this would be a nice add and would benefit our users.

rstoyanchev commented 2 years ago

There was another request #24236 for a new type of ApplicationContext that was declined recently, but this one comes from a different angle but I'm wondering how common the need is. I'll move it into the triage queue for consideration.

jxblum commented 2 years ago

Thank you @rstoyanchev.

For sure, I am uncertain how common the need is as I only had a single request arise in SBDG (using Apache Geode) for such an ApplicationContext. However, it would compliment the refreshability of an XML [Web] based ApplicationContext. As we know most users have switched to Spring Boot auto-configuration and use Annotation/Java based config to further customize their application configuration. To be clear, my thinking was simply that this would be a nice addition, but not strictly necessary if the team does not see the need or benefit. Just trying to give back to the core Spring Framework in whatever way I can.

rstoyanchev commented 2 years ago

Hi @jxblum, thanks for bringing this up. We've discussed it a bit this morning. It's a multi-faceted topic with a few dimensions. To start, the reasons for the present hierarchy are bit historical, but generally speaking the need for the refreshable aspect is declining, with most applications these days simply being restarted. We might even take the opportunity to simplify and make a few deprecations in this area, especially as it relates to support for older, web.xml style deployments.

On the XML config side, refreshability helps if the config is changed for example, but we were wondering what the actual case for refreshability that you've run into is. What actually needs or benefits from refreshing, or did you simply find yourself having to extend from AbstractRefreshableApplicationContext? If there is a link to an underlying issue or discussion on that, it would be great to link.

Overall, you can see we're not looking to expand the hierarchy, and there are some guidance we could provide on the implementation side and/or potential workarounds, but before we go there, it'd be great to learn about the case.

jxblum commented 2 years ago

Hi @rstoyanchev - Yes, I was actually trying to find the core Spring Framework team such a GitHub Issue ticket to refer to discussing the user's use case and requirements in more detail.

I have to apologize, but unfortunately, I interact with many users, customers and VMW field engineers over the course of an engagement, using many different mediums (Email, StackOverflow, Slack, Pivotal Tracker, the VMW customer commercial (JIRA?) issue ticketing system, etc), especially when it involves commercial GemFire customers.

I reviewed the Issue tickets in SBDG, both Open and Closed, and could not find a ticket discussing this use case. Sometimes, the information is customer sensitive and therefore only exists in VMW's customer issue tracking system. I will do some more digging.

Fortunately, I did add a comment to my RefreshableAnnotationConfigApplicationContext class that briefly describes what the user/customer was attempting to accomplish. This is very GemFire/Apache Geode specific.

In a nutshell, unlike other data store support we have in Spring [Data] (e.g. Redis, MongoDB, or even JDBC/JPA), it is possible with SDG, and by extension, SBDG, to configure and bootstrap servers, or rather "peer members" of a GemFire/Geode distributed system (cluster) using Spring [Boot]. This is to say, the Spring [Boot] application is actually a "peer member of the cluster" and will act as such.

In certain cases, although rare, a user might want their Spring Boot app to participate in the cluster as a peer member rather than simply be a "client" to the cluster (data affinity, functional capabilities, etc).

For any number of reasons, a GemFire/Geode peer can get kicked out of the cluster, for example when the member is sick (OOME, hardware failure). Usually, it is more likely the peer member might get kicked out due to a temporary network issues (glitches), for example the member is momentarily unreachable causing a potential split brain scenario. Basically, the peer member become "unresponsive".

When the peer member is forced out, it immediately tries to auto-reconnect (default behavior, and most commonly used by GemFire/Geode users and customers). Once reconnected, the old GemFire/Geode objects (cache, Regions, etc) are stale and the cache is rebuilt on that member, proceeding to rehydrate itself (inside a fence and merge process).

This means the necessarily "Singleton" Spring container beans representing the GemFire/Geode objects (cache, Regions, etc) are all now invalid.

I don't remember the users/customers' entire story, but they did not want to bring down the entire (monolithic) application, most likely because they were integrating with other transactional systems. They only wanted to refresh the GemFire/Geode objects.

I believe my experimental implementation of the RefreshableAnnotationConfigApplicationContext worked for them, though I rarely get all the details or any feedback to the effect. It is entirely possibly they went in a different direction in the end as well. I don't entirely know. You know how it is.

Anyway, I thought this to be an interesting enough use case to warrant further consideration at the core Spring Framework level. Additionally, it was not totally out of the ordinary given we had similar implementations for XML [Web] based ApplicationContexts, even if an application's configuration is externally managed, as is the case with XML.

Sorry for the long winded details. I am happy to answer additional questions in a meeting if you prefer. I am also equally happy this was acknowledged and if it ultimately gets turned down, I am fine with that. I just wanted to present the option since I did some initial ground work in this area already.

Regards, -j

rstoyanchev commented 2 years ago

Thanks for the extra details.

After a further discussion, this isn't something we plan to take on. Context hierarchy consistency is not a goal in and of itself. To a degree, there is a reason for the way it currently is, given that reloading Java config is not expected to provide new config. If anything we plan to scale down in this area rather expand the hierarchy further.

One idea discussed was closing and re-creating the context as a whole, which may or may not apply depending on what manages and uses the context. Alternatively, treating the entire app as a unit, and restart when needed is more likely what we would recommend. Apart from that, the rebuilding of caches and regions is arguably something that could be modelled and exposed as a feature in GemFire/Geode. The reloading of the entire ApplicationContext refresh for the same isn't something refreshable contexts were really meant for.

I believe @jhoeller will have some additional comments that will provide more guidance.

jhoeller commented 2 years ago

As Rossen mentioned, the refreshable feature in application contexts was designed for picking up freshly modified configuration files: within a running application, on the same class loader. Since this cannot usually happen for annotation-based configuration, we intentionally never introduced a regular refreshable annotation-based application context.

As a side note: We do have an AnnotationConfigWebApplicationContext variant, but the main reason there is compliance with the ConfigurableWebApplicationContext interface that we use for web.xml-driven configuration in a Servlet container. Refreshability is not the goal there, just alignment with the configuration style for XML-based web application contexts.

In terms of a reinitialization need for certain applications or certains parts of an application, our usual recommendation is to reboot at the deployment unit level: restarting the entire web application or the entire microservice, not refreshing an isolated Spring application context within a deployment unit that keeps running.

From a modern-day perspective, we are in the process of phasing out re-refreshable application contexts completely, at least outside of Servlet applications. We might even deprecate ClassPathXmlApplicationContext and FileSystemXmlApplicationContext soon.

Last but not least, for AOT processing in 6.0, we only support GenericApplicationContext variants that hold on to a fixed BeanFactory inside, without any re-refreshing support. This will even more strongly suggest GenericApplicationContext and AnnotationConfigApplicationContext as the common variants going forward.

rstoyanchev commented 2 years ago

Closing but thanks for raising and adding detail which can help others.