spring-projects / spring-framework

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

Propagate fully capable ServletContext in AbstractContextLoaderInitializer (for SessionCookieConfig access) #22319

Open vpavic opened 5 years ago

vpavic commented 5 years ago

Affects: 5.1.4.RELEASE

In Spring Session, we try to provide convenience of configuring our default CookieSerializer using Servlet API's SessionCookieConfig. The code involved can be seen here.

In a plain Spring app, without Spring Boot involved, this fails with Java based config due to inability to obtain SessionCookieConfig off injected ServletContext. However, the same action succeeds with XML based config.

I've put together a minimal sample app that exhibits this behavior.

Running XML config based sample using ./gradlew :sample-xml:tomcatRun will yield the following log output:

Jan 29, 2019 12:38:24 AM sample.Config setServletContext
INFO: Obtained SessionCookieConfig: org.apache.catalina.core.ApplicationSessionCookieConfig@5fbade79

While running Java config based sample using ./gradlew :sample-java:tomcatRun will result in the following error logged:

Jan 29, 2019 12:38:14 AM sample.Config setServletContext
WARNING: Cannot obtain SessionCookieConfig
java.lang.UnsupportedOperationException: Section 4.4 of the Servlet 3.0 specification does not permit this method to be called from a ServletContextListener that was not defined in web.xml, a web-fragment.xml file nor annotated with @WebListener
        at org.apache.catalina.core.StandardContext$NoPluggabilityServletContext.getSessionCookieConfig(StandardContext.java:6891)
        at sample.Config.setServletContext(Config.java:20)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at org.springframework.beans.factory.annotation.AutowiredAnnotationBeanPostProcessor$AutowiredMethodElement.inject(AutowiredAnnotationBeanPostProcessor.java:708)
        at org.springframework.beans.factory.annotation.InjectionMetadata.inject(InjectionMetadata.java:90)
        at org.springframework.beans.factory.annotation.AutowiredAnnotationBeanPostProcessor.postProcessProperties(AutowiredAnnotationBeanPostProcessor.java:374)
        at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.populateBean(AbstractAutowireCapableBeanFactory.java:1378)
        at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:575)
        at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:498)
        at org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:320)
        at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:222)
        at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:318)
        at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:199)
        at org.springframework.beans.factory.support.DefaultListableBeanFactory.preInstantiateSingletons(DefaultListableBeanFactory.java:846)
        at org.springframework.context.support.AbstractApplicationContext.finishBeanFactoryInitialization(AbstractApplicationContext.java:863)
        at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:546)
        at org.springframework.web.context.ContextLoader.configureAndRefreshWebApplicationContext(ContextLoader.java:400)
        at org.springframework.web.context.ContextLoader.initWebApplicationContext(ContextLoader.java:291)
        at org.springframework.web.context.ContextLoaderListener.contextInitialized(ContextLoaderListener.java:103)
        at org.apache.catalina.core.StandardContext.listenerStart(StandardContext.java:4898)
        at org.apache.catalina.core.StandardContext.startInternal(StandardContext.java:5363)
        at org.apache.catalina.util.LifecycleBase.start(LifecycleBase.java:145)
        at org.apache.catalina.core.ContainerBase$StartChild.call(ContainerBase.java:1410)
        at org.apache.catalina.core.ContainerBase$StartChild.call(ContainerBase.java:1400)
        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
        at java.lang.Thread.run(Thread.java:748)

This was originally reported against Spring Session in spring-projects/spring-session#1040.

sbrannen commented 5 years ago

Interesting... when the Servlet container is initialized via the infrastructure in AbstractAnnotationConfigDispatcherServletInitializer the ServletContext injected into your code is an instance of Tomcat's org.apache.catalina.core.StandardContext.NoPluggabilityServletContext which is documented as follows.

The wrapped version of the associated ServletContext that is presented to listeners that are required to have limited access to ServletContext methods. See Servlet 3.1 section 4.4.

That implementation always throws an UnsupportedOperationException for getSessionCookieConfig().

sbrannen commented 5 years ago

Thus, it would appear that getSessionCookieConfig() is officially unsupported when Spring's ContextLoaderListener is registered programmatically as in AbstractContextLoaderInitializer.registerContextLoaderListener(ServletContext).

sbrannen commented 5 years ago

Specifically, the following code from Tomcat's org.apache.catalina.core.StandardContext.listenerStart() method ensures that the Spring ContextLoaderListener receives a NoPluggabilityServletContext which wraps the real, underlying ServletContext.

        for (Object lifecycleListener: getApplicationLifecycleListeners()) {
            lifecycleListeners.add(lifecycleListener);
            if (lifecycleListener instanceof ServletContextListener) {
                noPluggabilityListeners.add(lifecycleListener);
            }
        }
sbrannen commented 5 years ago

I'm not yet sure what we can do here to make this work, but I've tentatively slated this for 5.2 for further investigation.

sbrannen commented 5 years ago

In the interim -- not that I'd really recommend it -- you could introduce a hack that works on that particular version of Tomcat by using reflection to access the underlying sc field in org.apache.catalina.core.StandardContext.NoPluggabilityServletContext.

    private static class NoPluggabilityServletContext
            implements ServletContext {

        private final ServletContext sc;
vpavic commented 5 years ago

Thanks for the feedback @sbrannen.

Unfortunately, since we're hitting this limitation in Spring Session configuration infrastructure, any workaround that involves specific Servlet Container classes is not viable.

I should have probably mentioned that the code involved also works in a Spring Boot app - I didn't include that in the sample repo though.

sbrannen commented 5 years ago

Unfortunately, since we're hitting this limitation in Spring Session configuration infrastructure, any workaround that involves specific Servlet Container classes is not viable.

Sure. That was actually intended to be more of a joke, which I should have stated explicitly. 😉

You'd obviously need a solution that works across containers and according to the Servlet spec.

I should have probably mentioned that the code involved also works in a Spring Boot app - I didn't include that in the sample repo though.

Interesting. In the sample you provided it appears that Tomcat is behaving according to the spec, so it might well be that the Spring Boot embedded Servlet container support registers the Spring ContextLoaderListener differently.

We'll have to take a look at what Spring Boot does in order to better understand the issue at hand.

Thanks for the feedback!

sbrannen commented 5 years ago

I'm not yet sure what we can do here to make this work, but I've tentatively slated this for 5.2 for further investigation.

I have unassigned the milestone and returned the status to "waiting for triage" in order for the team to assess the possibility of a such an enhancement.

vpavic commented 5 years ago

I've updated the minimal sample app repo with Spring Boot based sample and readme with relevant information on how to start each sample.

jastax commented 5 years ago

+1 :) People going through the Spring Session Redis guide will run into this

sandugadey commented 5 years ago

Any update on this , when the fix will be available??

Appropriate your help..

rstoyanchev commented 5 years ago

I don't know why a ServletContainerInitializer is allowed to use ServletContext config methods but a ServletContextListener registered from the ServletContainerInitializer isn't. Almost seems like an oversight in the spec, but be it as it may..

In the current situation, ServletContainerInitializer + ContextLoaderListener, I don't see much that can be done, and the embedded mode Boot sample isn't relevant either because it's not using a ServletContainerInitializer.

To make this work the AbstractDispatcherServletInitializer would probably have to replicate parts or all of ContextLoaderListener so that effectively Spring configuration is initialized within its onStartup method vs later when the container calls ContextLoaderListener#contextInitialized. Looking at Boot's SpringBootServletInitializer which is used in war deployment, it seems to do some such thing.

ghost commented 5 years ago

+1 :) People going through the Spring Session Redis guide will run into this

Anyone with Java Configuration will encounter this who uses Spring Session. I'm converting a non-Spring boot XML application to Java Configuration because of OAuth2Login configuration with XML namespace #4557 and ran into this issue.

So, either way I can't use both until I go to Spring Boot.

joerx commented 4 years ago

Seems manually creating a CookieSerializer somewhere in your config will work around the issue:

    @Bean
    public CookieSerializer cookieSerializer(ServletContext ctx) {
        logger.debug("Creating cookie serializer");
        DefaultCookieSerializer cs = new DefaultCookieSerializer();

        try {
            SessionCookieConfig cfg = ctx.getSessionCookieConfig();
            cs.setCookieName(cfg.getName());
            cs.setDomainName(cfg.getDomain());
            cs.setCookiePath(cfg.getPath());
            cs.setCookieMaxAge(cfg.getMaxAge());
        } catch (UnsupportedOperationException e) {
            cs.setCookieName("MY_SESSIONID");
            cs.setCookiePath(ctx.getContextPath());
        }

        return cs;
    }

This is because SpringHttpSessionConfiguration will only call getSessionCookieConfig to create a CookieSerializer when there isn't one already defined somewhere (see here)

xiaoshenxian commented 1 year ago

Any kind updates here? I encountered the same error with spring 6.0.10 and tomcat 10.1.12, and I also tried @joerx 's method, but the things just doesn't work, resulting my app cannot serialize sessions to redis.

Here is my code:

@Configuration
@EnableRedisHttpSession
public class SessionConfig {
    @Bean
    public CookieSerializer cookieSerializer(ServletContext ctx) {
        logger.debug("Creating cookie serializer");
        DefaultCookieSerializer cs=new DefaultCookieSerializer();

        try {
            SessionCookieConfig cfg=ctx.getSessionCookieConfig();
            cs.setCookieName(cfg.getName());
            cs.setDomainName(cfg.getDomain());
            cs.setCookiePath(cfg.getPath());
            cs.setCookieMaxAge(cfg.getMaxAge());
        } catch(UnsupportedOperationException e) {
            logger.debug("print the exception", e);
            cs.setCookieName("MY_SESSIONID");
            cs.setCookiePath(ctx.getContextPath());
        }

        return cs;
    }

    @Bean
    public JedisConnectionFactory connectionFactory() throws IOException {
        RedisStandaloneConfiguration config=new RedisStandaloneConfiguration(/*...*/);
        config.setPassword(/*...*/);

        JedisPoolConfig poolConfig=new JedisPoolConfig();
        poolConfig.setMaxTotal(/*...*/);
        poolConfig.setMaxIdle(/*...*/);
        poolConfig.setMaxWait(/*...*/);
        JedisClientConfiguration clientConfiguration=JedisClientConfiguration.builder()
                .connectTimeout(/*...*/)
                .readTimeout(/*...*/)
                .usePooling()
                .poolConfig(poolConfig)
                .build();

        return new JedisConnectionFactory(config, clientConfiguration);
    }
}

public class MyInitializer extends AbstractAnnotationConfigDispatcherServletInitializer {
    @Override
    protected Class<?>[] getRootConfigClasses() {
        return new Class[]{RootConfig.class, SessionConfig.class};
    }

    //...
}

I also tried to put @WebListener on the SessionConfig class, no effect observed.

If I didn't misremember, the code worked fine (without cookieSerializer Bean) on spring 5.3.6 and tomcat 9.0.52.

xiaoshenxian commented 1 year ago

I found an alternative config code instead of using @EnableRedisHttpSession, and it works fine for me.

That is just construct the SessionRepositoryFilter manually in the Initializer class

public class MvcWebApplicationInitializer extends AbstractAnnotationConfigDispatcherServletInitializer {
    @Override
    protected Filter[] getServletFilters() {
        RedisTemplate<String, Object> redisTemplate=new RedisTemplate<>();
        redisTemplate.setKeySerializer(RedisSerializer.string());
        redisTemplate.setHashKeySerializer(RedisSerializer.string());
        JedisConnectionFactory connectionFactory=SessionConfig.connectionFactory(); // construct your redis connection factory anywhere else
        connectionFactory.afterPropertiesSet();
        redisTemplate.setConnectionFactory(connectionFactory);
        redisTemplate.afterPropertiesSet();
        SessionRepositoryFilter<?> sessionRepositoryFilter=new SessionRepositoryFilter<>(new RedisSessionRepository(redisTemplate));

        // config other filters

        return new Filter[]{sessionRepositoryFilter/*, otherFilters*/};
    }

    //...
}

This works fine for me, but as I am using JedisConnectionFactory which has a destroy() method seems to release its resources, I don't know if it is necessary to call this when shutting down the server, and I retained the factory reference and invoke the destory() in my listener's contextDestroyed method anyway.

jhoeller commented 8 months ago

It looks like AbstractContextLoaderInitializer could try to pass its onStartup-provided ServletContext reference to the ContextLoaderListener that it registers, for use instead of the ServletContextEvent-provided reference. Or alternatively, initialize the WebApplicationContext right away like Boot does it. In any case, let's try to address this in 6.2 one way or the other.