CachingConnectionFactory - Invalid session in session cache [SPR-16450]

[spring-projects-issues]

Radek Kraus opened SPR-16450 and commented

I have a suspicion there is a small possibility, that invalid session (session, which belongs to the "old" connection) is returned into session cache (cachedSessions map). Let suppose following scenario.

Two threads - T1 and T2. T1 asks for connection and session and then close the acquired session (session is put into the session cache).

CachingConnectionFactory ccf = ...
Connection con1 = ccf.createConnection();
Session session1 = con1.createSession(false, Session.AUTO_ACKNOWLEDGE);
session1.close();

Now, the thread T2 invokes onException() method, because current connection is invalid (disconnection from JMS Provider or whatever ...).

ccf.onException(...)

The onException method caused that CachingConnectionFactory.resetConnection() is invoked (clear the session cache, active boolean flag settings). At the moment the thread T1 comes back and asks next JMS session (createSession(false, Session.AUTO_ACKNOWLEDGE)), exactly in a moment, when thread T2 already cleared the session cache and set active boolean flag back on true value (see my comment bellow). It causes that new session list is registered into cache session map (sessionList and it is passed into CachedSessionInvocationHandler - getSession() method).

@Override
public void resetConnection() {
    this.active = false;
    synchronized (this.cachedSessions) {
        for (LinkedList<Session> sessionList : this.cachedSessions.values()) {
	    synchronized (sessionList) {
	        for (Session session : sessionList) {
		    try {
		        session.close();
                    }
                    catch (Throwable ex) {
                        logger.trace("Could not close cached JMS Session", ex);
                    }
                }
            }
        }
	this.cachedSessions.clear();
    }
    this.active = true;
    // !!!
    // In this time thread T1 asks for next session => new sessionList is registered into cachedSessions
    // !!! 

    // Now proceed with actual closing of the shared Connection...
    super.resetConnection();
}

I know that invocation of createSession(...) method (done by T1) probably fails, because connection is already invalid, but we suppose that new session is successfully created. After T1 receives new session, the resetConnection() method is finished by invocation of super.resetConnection(). Now we suppose that thread T1 (or some another thread) invokes createConnection() method, which caused that new connection is established.

Now, the session, which was acquired by T1 in a moment when resetConnection() method was running, is closed => CachedSessionInvocationHandler.logicalClose() is invoked. Again, I know that there is a big probability, that this operation fails (message consumer close or session rolback throws JMSException, which is handled now - #16762). But when the "close" actions finish successfully, the session, which belong to "old" connection is returned back into session cache.

I attached "demonstration" test, which shows this behavior. Unfortunately I am unable to write this test, without modification of CachingConnectionFactory, because I need to simulate "some delay" in resetConnection() method, so as first I modified CachingConnectionFactory - new method clearSessionCache() was created (but this modification doesn't change the behavior).

Content of "demonstration" zip package:

• CachingConnectionFactory.java (modified CachingConnectionFactory, which is a start point for problem simulation)
• CachingConnectionFactoryTest.java (demonstration test itself)
• CachingConnectionFactory.java.diff (fix proposal)

I know that there is very small chance to reach this scenario (small time range, all close operations must be success ...), but anyway, I still feel "synchrozation" problem here. In addition my fix proposal is too "invasive" (signature of protected methods was changed ...). Maybe there is a better way how to fix this problem:

• clear session cache (again) in doCreateConnection() method
• move "clear" session cache operation into closeConnection() method, which is guarded by connectionMonitor lock (but it is not probably wanted - delay in create new connection till old connection is invalidated)
• ...

The main motivation for creating this issue was, that we have strange problems in old Spring (3.0.x), which can be caused by this problem. Maybe/probably the situation should be better from a moment when #16762 was done. But I think, that "synchronization" problem still exists.

---

Affects: 4.3.13

Attachments:

• CachingConnectionFactory.java (20.78 kB)
• demonstration-test.zip (9.48 kB)

Issue Links:

• CachingConnectionFactory should catch exceptions on logical close [SPR-12148] #16762 CachingConnectionFactory should catch exceptions on logical close
• JMS Producers are cached even when the destination is a temporary queue causing a memory leak [SPR-16353] #20900 JMS Producers are cached even when the destination is a temporary queue causing a memory leak

Referenced from: commits b6ecfcf, d8a2672

Backported to: 4.3.15

[spring-projects-issues]

Juergen Hoeller commented

The simplest solution here could be to move the this.active = true; statement to the end of the resetConnection() implementation, in combination with getSession not creating cached sessions while this.active == false. This should prevent cache interference in such phases, falling back to a regular physical Session for any such intermediate calls, without any need for extended synchronization.

I'm about to commit this to master. Would be great if you could give it a try once available!

[spring-projects-issues]

Radek Kraus commented

It sounds good for me. I think, that short term fallback to a physical session for intermediate calls is OK.

I modified my test case and it seems it works as expected - using physical session instead of cached session for intermediate calls (even in case, when session can be saved into session cache, because it was already created from "current" valid connection, but active flag is still false - very small time range between super.resetConnection() and this.active = true). I attached my version of CachingConnectionFactory, which was changed according your hint. Thank you.