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

CachingConnectionFactory too eagerly Session.commit [SPR-8437] #13083

Closed
spring-projects-issues opened this issue Jun 10, 2011 · 6 comments
Closed
Assignees
Labels
in: messaging Issues in messaging modules (jms, messaging) type: bug A general bug
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

spring-projects-issues commented Jun 10, 2011

Sven Zethelius opened SPR-8437 and commented

I set up a simple derived class from CachingConnectionFactory that proxied the Session so I could println trace the method calls made. In the process, I discovered a bug in the handling of transacted.

My use case is using Camel with spring-jms and spring-tx transactions. Exceptions cause the JMS message to be retried by the configuration of my ConnectionFactory, when transactions are enabled.

Actual:
When the broker is idle with no traffic, I'm seeing the following trace repeated until the session is in use again:
Invoke: boolean javax.jms.Session.getTransacted([]) @73850539
Invoke: void javax.jms.Session.commit([]) @73850539
Invoke: void javax.jms.Session.commit([]) @1538737979
Invoke: boolean javax.jms.Session.getTransacted([]) @73850539
Invoke: void javax.jms.Session.commit([]) @73850539
Invoke: void javax.jms.Session.commit([]) @1538737979

Expected:
Session should not be getting errant commit messages when there is no outstanding messages being processed.

The problem is because the if statements in org.springframework.jms.connection.CachingConnectionFactory.CachedSessionInvocationHandler.invoke(Object, Method, Object[])
give getTransacted the same behavior as the create* methods.
The call in org.springframework.jms.listener.AbstractMessageListenerContainer.commitIfNecessary(Session, Message) then calls getTransacted, marking the session as transactionOpen because it thinks a create* method was called.

fix:
I believe the fix is to conditionalize this.transactionOpen = true only on a create method, or possibly also recover, since it appears the invoke is missing handling for getTransacted, getMessageListener/setMessageListener, getAcknoledgeMode, recover, and unsubscribe.

{code:title bean setup}
<bean id="jmsServer" class="org.apache.camel.component.jms.JmsComponent">
<property name="configuration">
<bean class="com.expedia.cc.container.remoting.prototype.camel.JmsConfiguration">
<property name="connectionFactory" ref="jmsConnectionFactory.single" />
</bean>
</property>
<property name="transacted" value="true" />
<property name="useMessageIDAsCorrelationID" value="true" />
</bean>

<camel:camelContext trace="true" id="camelContext" >
</camel:camelContext>


<bean id="jmsTransactionManager" class="org.springframework.jms.connection.JmsTransactionManager">
    <property name="connectionFactory" ref="jmsConnectionFactory.single"/>
</bean>

<bean id="jmsConnectionFactory.single"
class="com.expedia.cc.container.remoting.prototype.DebugCachingConnectionFactory">
<property name="targetConnectionFactory" ref="jmsConnectionFactory.real" />
<property name="reconnectOnException" value="true" />
<property name="clientId" value="svenzPrototypeJMSAppClientID" />
</bean>

Affects: 3.0.5

Issue Links:

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

Good catch! Fixed for 3.1 RC1 as well as the upcoming 3.0.6 release, only setting transactionOpen=true for create* calls (since a recover call on its own does not open a transaction unless some actual listening operations happened before).

Juergen

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

Actually, before this fix, this shouldn't have caused additional commit calls but rather additional rollbacks calls (as triggered by logicalClose)...

Juergen

@spring-projects-issues
Copy link
Collaborator Author

Sven Zethelius commented

The commit calls come from the following in org.springframework.jms.listener.AbstractPollingMessageListenerContainer.

noMessageReceived(invoker, sessionToUse);
// Nevertheless call commit, in order to reset the transaction timeout (if any).
// However, don't do this on Tibco since this may lead to a deadlock there.
if (shouldCommitAfterNoMessageReceived(sessionToUse)) {
     commitIfNecessary(sessionToUse, message);
}

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

Hmm, such commits will still go through, even with yesterday's CachingConnectionFactory change. The 'transactionOpen' flag is only really used for a rollback cleanup decision in logicalClose... In all other respects, it's up to the caller to decide whether and when to call commit - just like with a plain (non-cached) JMS Session.

Juergen

@spring-projects-issues
Copy link
Collaborator Author

Sven Zethelius commented

Yeah, looking at this, I wouldn't be opposed to a "won't fix" or "by design" on this bug. Sorry for the mixup.

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

Actually, this bug is entirely worth fixing, and already fixed anyway :-) After all, we shouldn't trigger pointless rollbacks either, even if we can't do much about commit calls coming from a client.

So from the perspective of the commit log that you're seeing, this is rather something to tune in DefaultMessageListenerContainer itself. The default behavior will have to be commit calls even after no message received though, since otherwise the transaction timeout won't get reset properly on some message brokers. The sole exception that we're making at the moment is Tibco, preventing potential deadlocks there (and knowing that it doesn't enforce any kind of transaction timeout).

Juergen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: messaging Issues in messaging modules (jms, messaging) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

2 participants