CachingConnectionFactory does not rollback on Session.close() [SPR-5031]

[spring-projects-issues]

Joel Caplin opened SPR-5031 and commented

The JMS spec states that: "Closing a transacted session must roll back the transaction in progress": http://java.sun.com/javaee/5/docs/api/javax/jms/Session.html#close().

The CachingConnectionFactory does not appear to adhere to this rule. When a locally-transaction Session object is created using the cached Connection, calls to close() do not result in rollback() being called. Test case attached.

At runtime this can cause unexpected behavior. For example: I use a cached session and close it out, expecting an implicit rollback; a few seconds later, unbeknowst to me, I'm using the same cached session again, except I commit this time. The first implied rollback never happens so everything that took part in the first 'session' is actually commited.

---

Affects: 2.5.5

Attachments:

• CachingConnectionFactoryTest.java (2.21 kB)
• CachingConnectionFactoryTests.java (4.69 kB)

Issue Links:

• CachingConnectionFactory should catch exceptions on logical close [SPR-12148] #16762 CachingConnectionFactory should catch exceptions on logical close

[spring-projects-issues]

Joel Caplin commented

Test case demonstrating problem.

[spring-projects-issues]

Juergen Hoeller commented

Thanks for pointing this out! I've revised this for Spring 2.5.6: CachingConnectionFactory rolls back cached transacted JMS Sessions on logical close if no commit/rollback was issued. CachingConnectionFactory also explicitly closes cached JMS Sessions on Connection close now.

This will be available in tonight's 2.5.6 snapshot (http://static.springframework.org/downloads/nightly/snapshot-download.php?project=SPR). Please give it a try and let me know whether it works for you...

Juergen

[spring-projects-issues]

Joel Caplin commented

Hi Jurgen,

Thanks for the quick response.

I'll start with an observation:

The behavior of the fix seems a little inconsistent: if I create a connection, open up a session against it and immediately close that session, it rolls back. If I then immediately open a second session and close it, no roll back happens because the transactionCompleted flag is set to true.

Is this a necessary optimization? For example, if I were to call a method which won't change the state of the session's transaction, such as getAcknowledgeMode(), then the transactionCompleted gets set to false and the session rolls back on close.

Then move on to what I think is a bug:

I took your comment on board about the connection close() method making sure all the sessions under it are also logically closed out and couldn't get this behavior to work with the snapshot SingleConnectionFactory/CachingConnectionFactory from the July 23 nightly snapshot.

I've attached a modified test suite ("CachingConnectionFactoryTests") which includes testing rollback on closing of session (I fudged in session.getAcknowledgeMode() so that rollback would definitely be called). This works fine. The second test does not call session.close(), instead closing the connection owning the sessions; they don't appear to be rolled back even tho they think they're in the middle of a transaction (i.e. transactionCompleted is false).

Let me know your thoughts if you have time. Many thanks.

[spring-projects-issues]

Joel Caplin commented

(Also, apologies for my mis-spelling of your first name in the post above. Feel free to call me Jol.)

[spring-projects-issues]

Juergen Hoeller commented

Hi Joel,

Indeed, such a Session that was obtained and closed without any operations inbetween should consistently be returned to the pool without an implicit rollback. The main reason for the rollback optimization is regular JMS Session usage where an explicit commit() or rollback() call is done right before close(); in that case, we wouldn't want to additionally call rollback() on close. This is just a pretty simple check; as you noticed, any interaction with the Session does actually cause an implicit rollback on close - even if the interaction was just getAcknowledgeMode or the like. We could optimize this further, of course, but it would not show much further effect in typical usage. Handling the no-op-after-explicit-commit/rollback-case should be good enough there.

What I meant with the Session closing on Connection close was actually on shutdown of the CachingConnectionFactory, closing its shared physical Connection. Before we actually close the latter, we'll explicitly close all Sessions now (which had a bug that I just fixed where it might not actually have closed all Sessions). A logical close() call on the given Connection handle currently doesn't do anything special since it is operating on a shared Connection proxy. Unfortunately such a shared proxy cannot track the Session handles that a specific client opened, so those are not automatically closed there. As a consequence, CachingConnectionFactory strictly expects clients to actually close the Session handles as well, not just the Connection handle. We'd have to switch to per-client Connection proxies that just happen to use a shared physical Connection in the background in order to track Session handles per client... at the expense of proxy creation for each and every Connection handle obtained!

This sort of limitation is not uncommon with pools: JDBC connection pools typically require clients to close Statement handles for the same reason; otherwise result set cursors etc might remain open even after returning the Connection to the pool...

Juergen

(No worries about the spelling of my name, BTW. It's actually Jürgen - let's see whether JIRA can cope with special characters... The "ue" part is the international spelling of the umlaut character. Happy to call you Jol if you prefer, but I guess I'll stick with Joel :-)

[spring-projects-issues]

Juergen Hoeller commented

I've committed a revision for both consistent rollback-on-close behavior and consistent closing of all Sessions on shutdown of the CachingConnectionFactory. These changes will be available in tonight's snapshot.

The strict requirement to close Session handles explicitly, not relying on Connection.close() only, remains for the time being. (Spring's own JMS access code is consistent in closing Sessions explicitly already.)

Thanks a lot for the immediate testing!

Juergen

[spring-projects-issues]

Joel Caplin commented

Thank you sir, this is great.