From 5a9e75e1e6f834bc0b99a9b332dc634c5fa98717 Mon Sep 17 00:00:00 2001 From: Kai Inkinen Date: Tue, 5 May 2015 09:56:42 +0300 Subject: [PATCH 1/4] RedisSessionManager now uses different values as external and internal session id * Redis stores the id without the jvmRoute, since having it as part of the key effectively makes the session valid only on node with this jvmRoute * Customer still gets the original session id, including the jvmRoute, since this value is used for load balancing e.g. in mod_jk --- .../redissessions/RedisSessionManager.java | 25 ++++----- .../RedisSessionManagerTest.java | 56 +++++++++++++++++++ 2 files changed, 68 insertions(+), 13 deletions(-) create mode 100644 src/test/java/com/orangefunction/tomcat/redissessions/RedisSessionManagerTest.java diff --git a/src/main/java/com/orangefunction/tomcat/redissessions/RedisSessionManager.java b/src/main/java/com/orangefunction/tomcat/redissessions/RedisSessionManager.java index 2b58a261..f8973761 100644 --- a/src/main/java/com/orangefunction/tomcat/redissessions/RedisSessionManager.java +++ b/src/main/java/com/orangefunction/tomcat/redissessions/RedisSessionManager.java @@ -10,9 +10,6 @@ import org.apache.catalina.Session; import org.apache.catalina.session.ManagerBase; -import org.apache.commons.pool2.impl.GenericObjectPoolConfig; -import org.apache.commons.pool2.impl.BaseObjectPoolConfig; - import redis.clients.util.Pool; import redis.clients.jedis.JedisPool; import redis.clients.jedis.JedisSentinelPool; @@ -22,7 +19,6 @@ import java.io.IOException; import java.util.Arrays; -import java.util.Collections; import java.util.Enumeration; import java.util.Set; import java.util.EnumSet; @@ -341,17 +337,20 @@ public Session createSession(String requestedSessionId) { Jedis jedis = null; try { jedis = acquireConnection(); - + String redisSessionIdKey = null; // Ensure generation of a unique session identifier. if (null != requestedSessionId) { - sessionId = sessionIdWithJvmRoute(requestedSessionId, jvmRoute); - if (jedis.setnx(sessionId.getBytes(), NULL_SESSION) == 0L) { + sessionId = requestedSessionId; + + redisSessionIdKey = stripJvmRoute(requestedSessionId, jvmRoute); + if (jedis.setnx(redisSessionIdKey.getBytes(), NULL_SESSION) == 0L) { sessionId = null; } } else { do { - sessionId = sessionIdWithJvmRoute(generateSessionId(), jvmRoute); - } while (jedis.setnx(sessionId.getBytes(), NULL_SESSION) == 0L); // 1 = key set; 0 = key already existed + sessionId = generateSessionId(); + redisSessionIdKey = stripJvmRoute(sessionId, jvmRoute); + } while (jedis.setnx(redisSessionIdKey.getBytes(), NULL_SESSION) == 0L); // 1 = key set; 0 = key already existed } /* Even though the key is set in Redis, we are not going to flag @@ -396,10 +395,10 @@ This ensures that the save(session) at the end of the request return session; } - private String sessionIdWithJvmRoute(String sessionId, String jvmRoute) { - if (jvmRoute != null) { - String jvmRoutePrefix = '.' + jvmRoute; - return sessionId.endsWith(jvmRoutePrefix) ? sessionId : sessionId + jvmRoutePrefix; + String stripJvmRoute(String sessionId, String jvmRoute) { + if (jvmRoute != null && sessionId != null) { + final int index = sessionId.indexOf('.'); + sessionId = index != -1 ? sessionId.substring(0, index) : sessionId; } return sessionId; } diff --git a/src/test/java/com/orangefunction/tomcat/redissessions/RedisSessionManagerTest.java b/src/test/java/com/orangefunction/tomcat/redissessions/RedisSessionManagerTest.java new file mode 100644 index 00000000..fc909303 --- /dev/null +++ b/src/test/java/com/orangefunction/tomcat/redissessions/RedisSessionManagerTest.java @@ -0,0 +1,56 @@ +package com.orangefunction.tomcat.redissessions; + +import org.junit.Before; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +/** + * User: Kai Inkinen + * Date: 2015.05.04 + * Time: 10:15 + */ +public class RedisSessionManagerTest { + + private RedisSessionManager manager; + private String[] randomJvmRoutes = new String[]{"abc", "123", "abc123", "......", "1.1.2.jvmRoute", ",.,..,.,"}; + + @Before + public final void setupRedisSessionManagerTest() { + manager = new RedisSessionManager(); + } + + @Test + public void sessionIdWithoutJvmRouteIsReturned() { + assertEquals("session", manager.stripJvmRoute("session", "jvmRoute")); + } + + @Test + public void nullReturnedAsIs() { + assertNull(manager.stripJvmRoute(null, "jvmRoute")); + assertNull(manager.stripJvmRoute(null, null)); + } + + @Test + public void sessionIdIsNotModifiedIfWeDontHaveAJvmRoute() { + for (String jvmRoute : randomJvmRoutes) { + assertEquals("session." + jvmRoute, manager.stripJvmRoute("session." + jvmRoute, null)); + } + } + + @Test + public void sessionIdWithAnyJvmRouteIsModified() { + for (String jvmRoute : randomJvmRoutes) { + assertEquals("session", manager.stripJvmRoute("session." + jvmRoute, "unknown" + jvmRoute)); + } + } + + @Test + public void generatedSessionStripsJvmRoute() { + for (String jvmRoute : randomJvmRoutes) { + assertEquals("session", manager.stripJvmRoute("session." + jvmRoute, jvmRoute)); + } + } + +} From e438a1c565a1fe1e575f5dea493639124052a920 Mon Sep 17 00:00:00 2001 From: Kai Inkinen Date: Tue, 5 May 2015 09:57:23 +0300 Subject: [PATCH 2/4] Idea project files ignored --- .gitignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitignore b/.gitignore index 982a41f6..bb43513a 100644 --- a/.gitignore +++ b/.gitignore @@ -5,4 +5,6 @@ example-app/.gradle/* .DS_Store .rspec *.iml +*.ipr +*.iws .idea/* From 46cc72ecc8347cc04b023ee03b0224119fca5283 Mon Sep 17 00:00:00 2001 From: Kai Inkinen Date: Thu, 7 May 2015 17:37:17 +0300 Subject: [PATCH 3/4] Use the internal representation of the key in all queries to redis --- .../tomcat/redissessions/RedisSessionManager.java | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/orangefunction/tomcat/redissessions/RedisSessionManager.java b/src/main/java/com/orangefunction/tomcat/redissessions/RedisSessionManager.java index f8973761..8653edda 100644 --- a/src/main/java/com/orangefunction/tomcat/redissessions/RedisSessionManager.java +++ b/src/main/java/com/orangefunction/tomcat/redissessions/RedisSessionManager.java @@ -498,10 +498,11 @@ public byte[] loadSessionDataFromRedis(String id) throws IOException { Boolean error = true; try { - log.trace("Attempting to load session " + id + " from Redis"); + final String redisSessionIdKey = stripJvmRoute(id, getJvmRoute()); + log.trace("Attempting to load session " + id + " with key " + redisSessionIdKey + " from Redis"); jedis = acquireConnection(); - byte[] data = jedis.get(id.getBytes()); + byte[] data = jedis.get(redisSessionIdKey.getBytes()); error = false; if (data == null) { @@ -582,15 +583,18 @@ protected boolean saveInternal(Jedis jedis, Session session, boolean forceSave) RedisSession redisSession = (RedisSession)session; + final String sessionId = redisSession.getId(); + final String redisStorageKey = stripJvmRoute(sessionId, getJvmRoute()); + if (log.isTraceEnabled()) { - log.trace("Session Contents [" + redisSession.getId() + "]:"); + log.trace("Session Contents [" + sessionId + "]:"); Enumeration en = redisSession.getAttributeNames(); while(en.hasMoreElements()) { log.trace(" " + en.nextElement()); } } - byte[] binaryId = redisSession.getId().getBytes(); + final byte[] binaryId = redisStorageKey.getBytes(); Boolean isCurrentSessionPersisted; SessionSerializationMetadata sessionSerializationMetadata = currentSessionSerializationMetadata.get(); @@ -622,7 +626,7 @@ protected boolean saveInternal(Jedis jedis, Session session, boolean forceSave) log.trace("Save was determined to be unnecessary"); } - log.trace("Setting expire timeout on session [" + redisSession.getId() + "] to " + getMaxInactiveInterval()); + log.trace("Setting expire timeout on session [" + sessionId + "] to " + getMaxInactiveInterval()); jedis.expire(binaryId, getMaxInactiveInterval()); error = false; From 2cc7d09acda350c6220daa9632d529722657d771 Mon Sep 17 00:00:00 2001 From: Kai Inkinen Date: Tue, 12 May 2015 21:43:25 +0300 Subject: [PATCH 4/4] Improved logging in case of exceptions * Don't just log the exception, but also include the session id being handled at the moment * Log the action we're undertaking (add/remove) --- .../redissessions/RedisSessionManager.java | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/orangefunction/tomcat/redissessions/RedisSessionManager.java b/src/main/java/com/orangefunction/tomcat/redissessions/RedisSessionManager.java index 8653edda..b9c8bdc5 100644 --- a/src/main/java/com/orangefunction/tomcat/redissessions/RedisSessionManager.java +++ b/src/main/java/com/orangefunction/tomcat/redissessions/RedisSessionManager.java @@ -380,7 +380,7 @@ This ensures that the save(session) at the end of the request try { error = saveInternal(jedis, session, true); } catch (IOException ex) { - log.error("Error saving newly created session: " + ex.getMessage()); + log.error(String.format("Error saving newly created session [%s]: %s", sessionId, ex.getMessage()), ex); currentSession.set(null); currentSessionId.set(null); session = null; @@ -413,7 +413,7 @@ public void add(Session session) { try { save(session); } catch (IOException ex) { - log.warn("Unable to add to session manager store: " + ex.getMessage()); + log.warn("Unable to add to session manager store: " + ex.getMessage(), ex); throw new RuntimeException("Unable to add to session manager store.", ex); } } @@ -522,7 +522,7 @@ public DeserializedSessionContainer sessionFromSerializedData(String id, byte[] if (Arrays.equals(NULL_SESSION, data)) { log.error("Encountered serialized session " + id + " with data equal to NULL_SESSION. This is a bug."); - throw new IOException("Serialized session data was equal to NULL_SESSION"); + throw new IOException(String.format("Serialized session data for the session id [%s] was equal to NULL_SESSION", id)); } RedisSession session = null; @@ -633,8 +633,7 @@ protected boolean saveInternal(Jedis jedis, Session session, boolean forceSave) return error; } catch (IOException e) { - log.error(e.getMessage()); - + log.error(String.format("Exception encountered when performing saveInternal with the key [%s]", session.getId()), e); throw e; } finally { return error; @@ -667,8 +666,11 @@ public void remove(Session session, boolean update) { public void afterRequest() { RedisSession redisSession = currentSession.get(); if (redisSession != null) { + final String sessionId = redisSession.getId(); + final boolean validSession = redisSession.isValid(); + try { - if (redisSession.isValid()) { + if (validSession) { log.trace("Request with session completed, saving session " + redisSession.getId()); save(redisSession, getAlwaysSaveAfterRequest()); } else { @@ -676,7 +678,7 @@ public void afterRequest() { remove(redisSession); } } catch (Exception e) { - log.error("Error storing/removing session", e); + log.error(String.format("Error %s session with the id [%s]", validSession ? "storing" : "removing", sessionId), e); } finally { currentSession.remove(); currentSessionId.remove(); @@ -706,7 +708,7 @@ private void initializeDatabaseConnection() throws LifecycleException { connectionPool = new JedisPool(this.connectionPoolConfig, getHost(), getPort(), getTimeout(), getPassword()); } } catch (Exception e) { - e.printStackTrace(); + log.error("Exception while initializing JedisPool", e); throw new LifecycleException("Error connecting to Redis", e); } }