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

@JmsListener endpoint @Payload resolution fails due to interplay of new LazyResolutionMessage and MessagingMessageConverterAdapter [SPR-14389] #18962

Closed
spring-issuemaster opened this issue Jun 22, 2016 · 4 comments
Assignees
Milestone

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Jun 22, 2016

Eric Milles opened SPR-14389 and commented

When migrating to Spring 4.3.0, we found an incompatibility between our current @JmsListener config and the new lazy message resolution change 31a3607). When a message comes in, the PayloadArgumentResolver is trying to handle it. PAR.resolveArgument receives a LazyResolutionMessage. Calling getPayload delegates to an overridden extractPayload in MessagingMessageConverterAdapter. MMCA.extractPayload delegates to our configured MessagingMessageConverter's fromMessage. However, MMC.fromMessage has been overridden and so another LazyResolutionMessage is produced, instead of the converted payload.

As a workaround, I overrode MessagingMessageConverter.fromMessage to call its extractPayload. In Spring 4.2.6, I did not need this.

We have a JMS listener method like this:

   @JmsListener(id = "1", containerFactory = "jmsListenerContainerFactory", destination = "theEventsQueue")
    public void processResearchTrailEvents(@Payload final Event[] events, @Headers final Map<String, Object> headers)

@Configuration @EnableJms
public class MessagingConfig
{
    @Bean @Scope("prototype")
    public DefaultJmsListenerContainerFactory jmsListenerContainerFactory()
        throws Exception
    {
        final DefaultJmsListenerContainerFactory containerFactory = new DefaultJmsListenerContainerFactory();
        ...
        containerFactory.setMessageConverter(jmsMessageConverter());
        return containerFactory;
    }

    @Bean
    public MessageConverter jmsMessageConverter()
    {
        final MessagingMessageConverter messageConverter = new MessagingMessageConverter();
        messageConverter.setPayloadConverter(new EventsPayloadMessageConverter());
        return messageConverter;
    }
}

Affects: 4.3 GA

Referenced from: commits eba8730

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jun 29, 2016

Stéphane Nicoll commented

Thank you so much for the report. Changing the semantic of MessagingMessageConverter is an awful idea indeed :) Lazy payload resolution is an internal affair so we should really only change that in the internal implementation.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jun 29, 2016

Stéphane Nicoll commented

This is now fixed on master and a pseudo-project I've created with the code snippet you provided works fine now.

Now that I've gone to the actual detail of the problem, I have a few remarks about that code:

  1. The MessageConverter is meant to convert the javax.jms.Message to a payload, it isn't meant to customize the org.springframework.message.Message that is managed internally in the listener. It turns out that we allowed that construct by side effect in previous versions. I've added an additional logic so that we keep supporting that but you should really provide a MessageConverter that is focused on the payload (see below)
  2. Is there a reason why your factory has a prototype scope? That feels wrong for a factory who's used by the framework behind the scenes
  3. The jmsListenerContainerFactory bit in your listener is redudant as it's the default id. You may want to be explicit for some reason. In that case, I'd use a shorter id, something that makes sense for your app

In short, the following works (as well) and is what I'd recommend:

@JmsListener(id = "1", destination = "theEventsQueue")
public void processResearchTrailEvents(@Payload final Event[] events, @Headers final Map<String, Object> headers)

@Configuration @EnableJms
public class MessagingConfig
{
	@Bean 
	public DefaultJmsListenerContainerFactory jmsListenerContainerFactory()
			throws Exception
	{
		final DefaultJmsListenerContainerFactory containerFactory = new DefaultJmsListenerContainerFactory();
		...
		containerFactory.setMessageConverter(new EventsPayloadMessageConverter());
		return containerFactory;
	}

}
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 27, 2016

Eric Milles commented

Thanks for taking a look at this. I have no problems switching from setPayloadConverter to setMessageConverter; I just did what was necessary to get it working when I first integrated with the 4.1 @EnableJms and @JmsListener stuff. Thanks for all of that as well BTW. I was able to save a ton of config/boilerplate as I'm sure you're aware.

Re: Why prototype? We have multiple queue managers and queues. I define multiple @JmsListener tags to create more than one queue connection. In our case, each webapp has 2 MQ connections chosen at random from the pool. So if we start up several app servers, we should get balanced and redundant coverage of all the queues.

@Service
public class _Listener
{
    // declare two listeners so that two container factories are created and utilized
    @JmsListener(id = "1", containerFactory = "jmsListenerContainerFactory", destination = "theEventsQueue")
    @JmsListener(id = "2", containerFactory = "jmsListenerContainerFactory", destination = "theEventsQueue")
    public void process_Events(@Payload final Event[] events, @Headers final Map<String, Object> headers)
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 28, 2016

Stéphane Nicoll commented

probably the wrong place to have that discussion but I don't see why your factory has to be prototype. The factory is responsible to create actual MesssageListenerContainer instance. It can do so in that scenario with a default scope.

In other words, each JmsListener annotation is processed by a JmsListenerContainerFactory that will create a JmsMessageListenerContainer. The factory doesn't need to be prototype at all (it already takes care of creating a separate JmsMessageListenerContainer instance when it has to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.