Merge pull request #47 from jcoleman/save-policies

Save policies

Author: jcoleman

README.markdown

@@ -59,6 +59,7 @@ Add the following into your Tomcat context.xml (or the context block of the serv
              port="6379" <!-- optional: defaults to "6379" -->
              database="0" <!-- optional: defaults to "0" -->
              maxInactiveInterval="60" <!-- optional: defaults to "60" (in seconds) -->
+             sessionPersistPolicies="PERSIST_POLICY_1,PERSIST_POLICY_2,.." <!-- optional -->
              sentinelMaster="SentinelMasterName" <!-- optional -->
              sentinels="sentinel-host-1:port,sentinel-host-2:port,.." <!-- optional --> />
 
@@ -114,20 +115,15 @@ Then the example above would look like this:
     myArray.add(additionalArrayValue);
     session.setAttribute("customDirtyFlag");
 
+Persistence Policies
+--------------------
 
-Possible Issues
----------------
-
-There is the possibility of a race condition that would cause seeming invisibility of the session immediately after your web application logs in a user: if the response has finished streaming and the client requests a new page before the valve has been able to complete saving the session into Redis, then the new request will not see the session.
-
-This condition will be detected by the session manager and a java.lang.IllegalStateException with the message `Race condition encountered: attempted to load session[SESSION_ID] which has been created but not yet serialized.` will be thrown.
-
-Normally this should be incredibly unlikely (insert joke about programmers and "this should never happen" statements here) since the connection to save the session into Redis is almost guaranteed to be faster than the latency between a client receiving the response, processing it, and starting a new request.
+With an persistent session storage there is going to be the distinct possibility of race conditions when requests for the same session overlap/occur concurrently. Additionally, because the session manager works by serializing the entire session object into Redis, concurrent updating of the session will exhibit last-write-wins behavior for the entire session (not just specific session attributes).
 
-Possible solutions:
+Since each situation is different, the manager gives you several options which control the details of when/how sessions are persisted. Each of the following options may be selected by setting the `sessionPersistPolicies="PERSIST_POLICY_1,PERSIST_POLICY_2,.."` attributes in your manager declaration in Tomcat's context.xml. Unless noted otherwise, the various options are all combinable.
 
-- Enable the "save on change" feature by setting `saveOnChange` to `true` in your manager declaration in Tomcat's context.xml. Using this feature will degrade performance slightly as any change to the session will save the session synchronously to Redis, and technically this will still exhibit slight race condition behavior, but it eliminates as much possiblity of errors occurring as possible. __Note__: There's a tradeoff here in that if you make several changes to the session attributes over the lifetime of a long-running request while other short requests are making changes, you'll still end up having the long-running request overwrite the changes made by the short requests. Unfortunately there's no way to completely eliminate all possible race conditions here, so you'll have to determine what's necessary for your specific use case.
-- If you encounter errors, then you can force save the session early (before sending a response to the client) then you can retrieve the current session, and call `currentSession.manager.save(currentSession, true)` to synchronously eliminate the race condition. Note: this will only work directly if your application has the actual session object directly exposed. Many frameworks (and often even Tomcat) will expose the session in their own wrapper HttpSession implementing class. You may be able to dig through these layers to expose the actual underlying RedisSession instance--if so, then using that instance will allow you to implement the workaround.
+- `SAVE_ON_CHANGE`: every time `session.setAttribute()` or `session.removeAttribute()` is called the session will be saved. __Note:__ This feature cannot detect changes made to objects already stored in a specific session attribute. __Tradeoffs__: This option will degrade performance slightly as any change to the session will save the session synchronously to Redis.
+- `ALWAYS_SAVE_AFTER_REQUEST`: force saving after every request, regardless of whether or not the manager has detected changes to the session. This option is particularly useful if you make changes to objects already stored in a specific session attribute. __Tradeoff:__ This option make actually increase the liklihood of race conditions if not all of your requests change the session.
 
 Acknowledgements
 ----------------

example-app/src/main/java/com/orangefunction/tomcatredissessionmanager/exampleapp/WebApp.java

@@ -225,8 +225,8 @@ public Object handle(Request request, Response response) {
 
            RedisSessionManager manager = getRedisSessionManager(request);
            if (null != manager) {
-             if (key.equals("saveOnChange")) {
-               map.put("value", new Boolean(manager.getSaveOnChange()));
+             if (key.equals("sessionPersistPolicies")) {
+               map.put("value", manager.getSessionPersistPolicies());
              } else if (key.equals("maxInactiveInterval")) {
                map.put("value", new Integer(manager.getMaxInactiveInterval()));
              }
@@ -248,9 +248,9 @@ public Object handle(Request request, Response response) {
 
            RedisSessionManager manager = getRedisSessionManager(request);
            if (null != manager) {
-             if (key.equals("saveOnChange")) {
-               manager.setSaveOnChange(Boolean.parseBoolean(value));
-               map.put("value", new Boolean(manager.getSaveOnChange()));
+             if (key.equals("sessionPersistPolicies")) {
+               manager.setSessionPersistPolicies(value);
+               map.put("value", manager.getSessionPersistPolicies());
              } else if (key.equals("maxInactiveInterval")) {
                manager.setMaxInactiveInterval(Integer.parseInt(value));
                map.put("value", new Integer(manager.getMaxInactiveInterval()));

spec/requests/sessions_spec.rb

@@ -102,13 +102,15 @@
     describe "change on save" do
 
       before :each do
-        get("#{SETTINGS_PATH}/saveOnChange")
-        @oldSaveOnChangeValue = json['value']
-        post("#{SETTINGS_PATH}/saveOnChange", body: {value: 'true'})
+        get("#{SETTINGS_PATH}/sessionPersistPolicies")
+        @oldSessionPersistPoliciesValue = json['value']
+        enums = @oldSessionPersistPoliciesValue.split(',')
+        enums << 'SAVE_ON_CHANGE'
+        post("#{SETTINGS_PATH}/sessionPersistPolicies", body: {value: enums.join(',')})
       end
 
       after :each do
-        post("#{SETTINGS_PATH}/saveOnChange", body: {value: @oldSaveOnChangeValue})
+        post("#{SETTINGS_PATH}/sessionPersistPolicies", body: {value: @oldSessionPersistPoliciesValue})
       end
 
       it 'should support persisting the session on change to minimize race conditions on simultaneous updates' do

src/main/java/com/radiadesign/catalina/session/RedisSessionHandlerValve.java

@@ -38,7 +38,7 @@ private void storeOrRemoveSession(Session session) {
           log.trace("Request with session completed, saving session " + session.getId());
           if (session.getSession() != null) {
             log.trace("HTTP Session present, saving " + session.getId());
-            manager.save(session);
+            manager.save(session, manager.getAlwaysSaveAfterRequest());
           } else {
             log.trace("No HTTP Session present, Not saving " + session.getId());
           }
@@ -48,7 +48,7 @@ private void storeOrRemoveSession(Session session) {
         }
       }
     } catch (Exception e) {
-      // Do nothing.
+      log.error("Error storing/removing session", e);
     }
   }
 }

src/main/java/com/radiadesign/catalina/session/RedisSessionManager.java

@@ -25,14 +25,31 @@
 import java.util.Collections;
 import java.util.Enumeration;
 import java.util.Set;
+import java.util.EnumSet;
 import java.util.HashSet;
+import java.util.Iterator;
 
 import org.apache.juli.logging.Log;
 import org.apache.juli.logging.LogFactory;
 
 
 public class RedisSessionManager extends ManagerBase implements Lifecycle {
 
+  enum SessionPersistPolicy {
+    DEFAULT,
+    SAVE_ON_CHANGE,
+    ALWAYS_SAVE_AFTER_REQUEST;
+
+    static SessionPersistPolicy fromName(String name) {
+      for (SessionPersistPolicy policy : SessionPersistPolicy.values()) {
+        if (policy.name().equalsIgnoreCase(name)) {
+          return policy;
+        }
+      }
+      throw new IllegalArgumentException("Invalid session persist policy [" + name + "]. Must be one of " + Arrays.asList(SessionPersistPolicy.values())+ ".");
+    }
+  }
+
   protected byte[] NULL_SESSION = "null".getBytes();
 
   private final Log log = LogFactory.getLog(RedisSessionManager.class);
@@ -43,7 +60,6 @@ public class RedisSessionManager extends ManagerBase implements Lifecycle {
   protected String password = null;
   protected int timeout = Protocol.DEFAULT_TIMEOUT;
   protected String sentinelMaster = null;
-  protected String sentinels = null;
   Set<String> sentinelSet = null;
 
   protected Pool<Jedis> connectionPool;
@@ -59,7 +75,7 @@ public class RedisSessionManager extends ManagerBase implements Lifecycle {
 
   protected String serializationStrategyClass = "com.radiadesign.catalina.session.JavaSerializer";
 
-  protected boolean saveOnChange = false;
+  protected EnumSet<SessionPersistPolicy> sessionPersistPoliciesSet = EnumSet.of(SessionPersistPolicy.DEFAULT);
 
   /**
    * The lifecycle event support for this component.
@@ -110,16 +126,45 @@ public void setSerializationStrategyClass(String strategy) {
     this.serializationStrategyClass = strategy;
   }
 
+  public String getSessionPersistPolicies() {
+    StringBuilder policies = new StringBuilder();
+    for (Iterator<SessionPersistPolicy> iter = this.sessionPersistPoliciesSet.iterator(); iter.hasNext();) {
+      SessionPersistPolicy policy = iter.next();
+      policies.append(policy.name());
+      if (iter.hasNext()) {
+        policies.append(",");
+      }
+    }
+    return policies.toString();
+  }
+
+  public void setSessionPersistPolicies(String sessionPersistPolicies) {
+    String[] policyArray = sessionPersistPolicies.split(",");
+    EnumSet<SessionPersistPolicy> policySet = EnumSet.of(SessionPersistPolicy.DEFAULT);
+    for (String policyName : policyArray) {
+      SessionPersistPolicy policy = SessionPersistPolicy.fromName(policyName);
+      policySet.add(policy);
+    }
+    this.sessionPersistPoliciesSet = policySet;
+  }
+
   public boolean getSaveOnChange() {
-    return saveOnChange;
+    return this.sessionPersistPoliciesSet.contains(SessionPersistPolicy.SAVE_ON_CHANGE);
   }
 
-  public void setSaveOnChange(boolean saveOnChange) {
-    this.saveOnChange = saveOnChange;
+  public boolean getAlwaysSaveAfterRequest() {
+    return this.sessionPersistPoliciesSet.contains(SessionPersistPolicy.ALWAYS_SAVE_AFTER_REQUEST);
   }
 
   public String getSentinels() {
-    return sentinels;
+    StringBuilder sentinels = new StringBuilder();
+    for (Iterator<String> iter = this.sentinelSet.iterator(); iter.hasNext();) {
+      sentinels.append(iter.next());
+      if (iter.hasNext()) {
+        sentinels.append(",");
+      }
+    }
+    return sentinels.toString();
   }
 
   public void setSentinels(String sentinels) {
@@ -129,8 +174,6 @@ public void setSentinels(String sentinels) {
 
     String[] sentinelArray = sentinels.split(",");
     this.sentinelSet = new HashSet<String>(Arrays.asList(sentinelArray));
-
-    this.sentinels = sentinels;
   }
 
   public Set<String> getSentinelSet() {
@@ -338,11 +381,14 @@ This ensures that the save(session) at the end of the request
       currentSessionId.set(sessionId);
       currentSessionIsPersisted.set(false);
 
-      if (null != session && this.getSaveOnChange()) {
+      if (null != session) {
         try {
-          save(session);
+          error = saveInternal(jedis, session, true);
         } catch (IOException ex) {
-          log.error("Error saving newly created session (triggered by saveOnChange=true): " + ex.getMessage());
+          log.error("Error saving newly created session: " + ex.getMessage());
+          currentSession.set(null);
+          currentSessionId.set(null);
+          session = null;
         }
       }
     } finally {
@@ -455,8 +501,6 @@ public RedisSession loadSessionFromRedis(String id) throws IOException {
       if (data == null) {
         log.trace("Session " + id + " not found in Redis");
         session = null;
-      } else if (Arrays.equals(NULL_SESSION, data)) {
-        throw new IllegalStateException("Race condition encountered: attempted to load session[" + id + "] which has been created but not yet serialized.");
       } else {
         log.trace("Deserializing session " + id + " from Redis");
         session = (RedisSession)createEmptySession();
@@ -499,10 +543,25 @@ public void save(Session session, boolean forceSave) throws IOException {
     Jedis jedis = null;
     Boolean error = true;
 
+    try {
+      jedis = acquireConnection();
+      error = saveInternal(jedis, session, forceSave);
+    } catch (IOException e) {
+      throw e;
+    } finally {
+      if (jedis != null) {
+        returnConnection(jedis, error);
+      }
+    }
+  }
+
+  protected boolean saveInternal(Jedis jedis, Session session, boolean forceSave) throws IOException {
+    Boolean error = true;
+
     try {
       log.trace("Saving session " + session + " into Redis");
 
-      RedisSession redisSession = (RedisSession) session;
+      RedisSession redisSession = (RedisSession)session;
 
       if (log.isTraceEnabled()) {
         log.trace("Session Contents [" + redisSession.getId() + "]:");
@@ -517,8 +576,6 @@ public void save(Session session, boolean forceSave) throws IOException {
       redisSession.resetDirtyTracking();
       byte[] binaryId = redisSession.getId().getBytes();
 
-      jedis = acquireConnection();
-
       Boolean isCurrentSessionPersisted = this.currentSessionIsPersisted.get();
       if (forceSave || sessionIsDirty || (isCurrentSessionPersisted == null || !isCurrentSessionPersisted)) {
         jedis.set(binaryId, serializer.serializeFrom(redisSession));
@@ -530,14 +587,14 @@ public void save(Session session, boolean forceSave) throws IOException {
       jedis.expire(binaryId, getMaxInactiveInterval());
 
       error = false;
+
+      return error;
     } catch (IOException e) {
       log.error(e.getMessage());
 
       throw e;
     } finally {
-      if (jedis != null) {
-        returnConnection(jedis, error);
-      }
+      return error;
     }
   }