Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attempt to support Spring Boot WebApplicationContext initialization and lookup #368

Closed
wants to merge 3 commits into from

Conversation

lincolnthree
Copy link
Member

Fixes #362

{

private final Logger log = Logger.getLogger(SpringServiceEnricher.class);
private static ServletContext context;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens, if this class runs in a traditionally deployed war file in a servlet container?

If there were multiple applications deployed, the ServletContext instances could overwrite each other and possibly leak into the wrong application?

Copy link
Member Author

@lincolnthree lincolnthree Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean in Servlet Container where someone has put shared dependencies in the servlet container classpath? Or in an EAR type container where JARs and dependencies are shared between WARs? I guess that could be a concern. Let me think about that. Good question.

Though I think a traditional WAR deployment where dependencies are provided in each WAR should not have any issues since each would have their own copy of the class files, unless I've forgotten how those work entirely.

Each WAR should have its own ClassLoader instance that should have the same parent AFAIK. (Unless it's a modular-classloaded servlet container, at which point neither would share a parent and they'd all be entirely independent, more or less.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exactly what I mean. "Simple" war deplyoments should be fine, when every war brings its own copy of rewrite which gets loaded by a dedicated classloader.

In more complex scenarios with some sort of shared classloading (EAR with jars and wars, or rewrite being deployed into the servlet-container itself) however, the static field could be a problem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok glad we are on the same page. I wasn't thinking of those use-cases since they aren't very common anymore, but I think you're right that they should be handled regardless. I'll work on it. Now that we have a framework for how to make this function it shouldn't be too hard. Any ideas are welcome :)

import org.springframework.web.context.support.SpringBeanAutowiringSupport;

/**
* {@link ServiceEnricher} implementation for Spring.
*
* @author Christian Kaltepoth
*/
public class SpringServiceEnricher implements ServiceEnricher
public class SpringServiceEnricher implements ServiceEnricher, ContextListener
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this setup, the SpringServiceEnricher will be instantiated twice, won't it? Once as ServiceEnricher and once as ContextListener

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but only one of them would be used as an Enricher. The other would just handle the static. I could split this into a different ContextListener implementation, and try to deal with the EAR / shared lib classloader issue there.

@lincolnthree
Copy link
Member Author

lincolnthree commented Mar 22, 2023

@larsgrefer Ok. Updated with what I think should resolve the issue. Let me know your thoughts! Actually I do have some concerns about part of this due to the fact that Threads might be re-used between ServletContexts depending on how containers do initialization, but interested in your thoughts.

@lincolnthree
Copy link
Member Author

Dangit. I did not mean to close this by renaming the branch. Working on a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants