spring-projects / spring-session

Spring Session
https://spring.io/projects/spring-session
Apache License 2.0
1.86k stars 1.11k forks source link

Using Spring Session Redis prevents you from creating your own bean named redisMessageListenerContainer #1252

Closed wilkinsona closed 5 years ago

wilkinsona commented 5 years ago

Via this Stack Overflow question, Spring Session Redis creates a bean named redisMessageListenerContainer that prevents a user from creating their own bean with that name. A bean will either get overridden or, if overriding is disabled as it is by default in Spring Boot 2.1, a startup failure will occur.

The quickest solution would be to name Spring Session's bean more specifically. redisMessageListenerContainer is a very generic name for a piece of Spring Session's infrastructure. I think it would also be interesting to consider solutions beyond that and take a look at whether it needs to be a RedisMessageListenerContainer bean at all. If the listener container is intended to be used solely by Spring Session, it would seem to make sense for it to not be exposed as a RedisMessageListenerContainer bean. If it's intended to be a general purpose component, then it would seem to make sense for Spring Session to be able to reuse an existing listener container rather than always creating its own.

rwinch commented 5 years ago

@wilkinsona Thanks for the report.

At the moment I think it makes the most sense to change the bean name to be prefixed with springSession.

I think it would also be interesting to consider solutions beyond that and take a look at whether it needs to be a RedisMessageListenerContainer bean at all. If the listener container is intended to be used solely by Spring Session, it would seem to make sense for it to not be exposed as a RedisMessageListenerContainer bean.

Can you elaborate on what you mean by "it would seem to make sense for it to not be exposed as a RedisMessageListenerContainer bean"?

I'm interpreting this as a suggestion for Spring Session to provide its own abstraction? If that is the case, I don't think we should do provide any abstractions in Spring Session. While the RedisMessageListenerContainer is intended to be for Spring Session, it is only for the Spring Data Redis implementation of Spring Session. To me adding an abstraction here would be like asking Spring Cache abstraction to provide abstractions for configuring the caches that they support. It doesn't make sense to abstract the configuration because different caches have different options.

vpavic commented 5 years ago

I think what @wilkinsona meant was if we could configure our RedisMessageListenerContainer manually, without registering it as @Bean in order for our infrastructure to be more application context friendly and reduce the probability of conflicts or overriding.

That would be similar to what we've done with RedisTemplate and JdbcTemplate in 2.0, where instances that we use in SessionRepository implementations are configured internally but not exposed to the context. Since RedisMessageListenerContainer implements InitializingBean we'd call #afterPropertiesSet by ourselves (again, same as with RedisTemplate) but since it also implements DisposableBean we'd have to take care of invoking #destroy too.

wilkinsona commented 5 years ago

Thanks, @vpavic. That's exactly what I meant.

When I (confusingly) said " it would seem to make sense for it to not be exposed as a RedisMessageListenerContainer bean", I was imagining a case where the listener container and other pieces of infrastructure still needed to be available for injection to other parts of Spring Session. In that case, a SpringSessionInfrastructure bean or similar could be used. It would hold all the internal pieces of infrastructure, manage their lifecycles via delegating as appropriate, and make them available indirectly via injection of the SpringSessionInfrastructure bean.

I'm not sufficiently familiar with Spring Session internals to know if it would be necessary to go to those lengths. From the description of what you've done with RedisTemplate it sounds like it may not be.

rwinch commented 5 years ago

Thanks for the clarifications.

I'd prefer to stick with Spring Data's contract which is exposing it as a Bean. Wrapping it seems to read into the implementation a bit too much. For example, if RedisMessageListenerContainer adds more lifecycle methods we would need to make sure we support those too. Another point is that I don't believe (correct me if I'm wrong) that exposing this as a Bean with a unique name is going to impact many users since they are not unlikely to autowire RedisMessageListenerContainer by type. This differs from RedisTemplate, for example, since users are likely to autowire it by type.

In short, unless we have sufficient reason to do otherwise I'd prefer to stick with exposing it as a Bean using a more unique bean name.

vpavic commented 5 years ago

Another point is that I don't believe (correct me if I'm wrong) that exposing this as a Bean with a unique name is going to impact many users since they are not unlikely to autowire RedisMessageListenerContainer by type. This differs from RedisTemplate, for example, since users are likely to autowire it by type.

I agree, this is a valid point. Prefixing the bean name springSession should address this without us having to add too much complexity.