Resolve threading issues for simultaneous session invalidate.

12 files changed, 356 insertions, 214 deletions

diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/SessionIdManager.java b/jetty-server/src/main/java/org/eclipse/jetty/server/SessionIdManager.java
index 13619af126..755b863a22 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/SessionIdManager.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/SessionIdManager.java
@@ -44,8 +44,9 @@ public interface SessionIdManager extends LifeCycle
     /**
      * Remove id
      * @param id the plain session id (no workername extension) of the session to remove
+     * @return true if the id was removed, false otherwise
      */
-    public void removeId (String id);
+    public boolean removeId (String id);
     
     /**
      * Invalidate all sessions on all contexts that share the same id.
diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionIdManager.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionIdManager.java
index 0a0f9e921e..aa5e723e3a 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionIdManager.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionIdManager.java
@@ -19,7 +19,9 @@
 package org.eclipse.jetty.server.session;
 
 import java.security.SecureRandom;
+import java.util.HashSet;
 import java.util.Random;
+import java.util.Set;
 
 import javax.servlet.http.HttpServletRequest;
 
@@ -322,47 +324,29 @@ public abstract class AbstractSessionIdManager extends AbstractLifeCycle impleme
     public void expireAll(String id)
     {
         //take the id out of the list of known sessionids for this node
-        removeId(id);
-
-        //tell all contexts that may have a session object with this id to
-        //get rid of them
-        Handler[] contexts = _server.getChildHandlersByClass(ContextHandler.class);
-        for (int i=0; contexts!=null && i<contexts.length; i++)
+        if (removeId(id))
         {
-            SessionHandler sessionHandler = ((ContextHandler)contexts[i]).getChildHandlerByClass(SessionHandler.class);
-            if (sessionHandler != null)
+            //tell all contexts that may have a session object with this id to
+            //get rid of them
+            for (SessionManager manager:getSessionManagers())
             {
-                SessionManager manager = (SessionManager)sessionHandler.getSessionManager();
-
-                if (manager != null)
-                {
-                    manager.invalidate(id);
-                }
+                manager.invalidate(id);
             }
         }
     }
-    
+
     public void invalidateAll (String id)
     {
         //take the id out of the list of known sessionids for this node
-        removeId(id);
-
-        //tell all contexts that may have a session object with this id to
-        //get rid of them
-        Handler[] contexts = _server.getChildHandlersByClass(ContextHandler.class);
-        for (int i=0; contexts!=null && i<contexts.length; i++)
+        if (removeId(id))
         {
-            SessionHandler sessionHandler = ((ContextHandler)contexts[i]).getChildHandlerByClass(SessionHandler.class);
-            if (sessionHandler != null)
-            {
-                SessionManager manager = (SessionManager)sessionHandler.getSessionManager();
-
-                if (manager != null)
-                {
-                    manager.invalidate(id);
-                }
-            }
-        }      
+            //tell all contexts that may have a session object with this id to
+            //get rid of them
+            for (SessionManager manager:getSessionManagers())
+            {         
+                manager.invalidate(id);
+            } 
+        }
     }
 
     /** 
@@ -373,9 +357,21 @@ public abstract class AbstractSessionIdManager extends AbstractLifeCycle impleme
     { 
         //generate a new id
         String newClusterId = newSessionId(request.hashCode());
-        removeId(oldClusterId);//remove the old one from the list
         
+        removeId(oldClusterId);//remove the old one from the list
+
         //tell all contexts to update the id 
+        for (SessionManager manager:getSessionManagers())
+        {
+            manager.renewSessionId(oldClusterId, oldNodeId, newClusterId, getExtendedId(newClusterId, request));
+        }
+    }
+    
+    
+    private Set<SessionManager> getSessionManagers()
+    {
+        Set<SessionManager> managers = new HashSet<>();
+
         Handler[] contexts = _server.getChildHandlersByClass(ContextHandler.class);
         for (int i=0; contexts!=null && i<contexts.length; i++)
         {
@@ -385,10 +381,9 @@ public abstract class AbstractSessionIdManager extends AbstractLifeCycle impleme
                 SessionManager manager = (SessionManager)sessionHandler.getSessionManager();
 
                 if (manager != null)
-                {
-                    manager.renewSessionId(oldClusterId, oldNodeId, newClusterId, getExtendedId(newClusterId, request));
-                }
+                    managers.add(manager);
             }
         }
+        return managers;
     }
 }
diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionStore.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionStore.java
index 08c33edcd1..cffb7e700d 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionStore.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionStore.java
@@ -27,6 +27,7 @@ import javax.servlet.http.HttpServletRequest;
 import org.eclipse.jetty.util.component.AbstractLifeCycle;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
+import org.eclipse.jetty.util.thread.Locker.Lock;
 
 /**
  * AbstractSessionStore
@@ -89,7 +90,7 @@ public abstract class AbstractSessionStore extends AbstractLifeCycle implements
      * @param id
      * @return true if removed false otherwise
      */
-    public abstract boolean doDelete (String id);
+    public abstract Session doDelete (String id);
     
     
     
@@ -193,7 +194,7 @@ public abstract class AbstractSessionStore extends AbstractLifeCycle implements
         
         if (staleCheck && isStale(session))
         {
-            //delete from store so should reload
+            //delete from session store so should reload from session data store
             doDelete(id);
             session = null;
         }
@@ -234,13 +235,26 @@ public abstract class AbstractSessionStore extends AbstractLifeCycle implements
         session.setSessionManager(_manager);
 
         //if the session is new, the data has changed, or the cache is considered stale, write it to any backing store
-        if ((session.isNew() || session.getSessionData().isDirty() || isStale(session)) && _sessionDataStore != null)
+        try (Lock lock = session.lock())
         {
-            if (_sessionDataStore.isPassivating())
-                session.willPassivate();
-            _sessionDataStore.store(id, session.getSessionData());
-            if (_sessionDataStore.isPassivating())
-                session.didActivate();
+            if ((session.isNew() || session.getSessionData().isDirty() || isStale(session)) && _sessionDataStore != null)
+            {
+                if (_sessionDataStore.isPassivating())
+                {
+                    session.willPassivate();
+                    try
+                    {
+                        _sessionDataStore.store(id, session.getSessionData());
+                    }
+                    finally
+                    {
+                        session.didActivate();
+                    }
+                }
+                else
+                    _sessionDataStore.store(id, session.getSessionData());
+            }
+
         }
 
         doPutIfAbsent(id,session);
@@ -264,7 +278,7 @@ public abstract class AbstractSessionStore extends AbstractLifeCycle implements
      * @see org.eclipse.jetty.server.session.SessionStore#delete(java.lang.String)
      */
     @Override
-    public boolean delete(String id) throws Exception
+    public Session delete(String id) throws Exception
     {
         if (_sessionDataStore != null)
         {
@@ -274,6 +288,12 @@ public abstract class AbstractSessionStore extends AbstractLifeCycle implements
         return doDelete(id);
     }
 
+    
+    
+    /**
+     * @param session
+     * @return
+     */
     public boolean isStale (Session session)
     {
         if (_staleStrategy != null)
diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/HashSessionIdManager.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/HashSessionIdManager.java
index 70d94259e4..bbabd9f764 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/HashSessionIdManager.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/HashSessionIdManager.java
@@ -52,16 +52,6 @@ public class HashSessionIdManager extends AbstractSessionIdManager
     }
 
 
-    /** 
-     * @see org.eclipse.jetty.server.session.AbstractSessionIdManager#newSessionId(long)
-     */
-    @Override
-    public String newSessionId(long seedTerm)
-    {
-        String id = super.newSessionId(seedTerm);
-        return id;
-    }
-    
     
     /** 
      * @see org.eclipse.jetty.server.SessionIdManager#useId(java.lang.String)
@@ -80,9 +70,9 @@ public class HashSessionIdManager extends AbstractSessionIdManager
      * @see org.eclipse.jetty.server.SessionIdManager#removeId(java.lang.String)
      */
     @Override
-    public void removeId(String id)
+    public boolean removeId(String id)
     {
-       _ids.remove(id);
+       return _ids.remove(id);
     }
 
 }
diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/JDBCSessionDataStore.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/JDBCSessionDataStore.java
index 89c2dd2839..d810c19c06 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/JDBCSessionDataStore.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/JDBCSessionDataStore.java
@@ -665,7 +665,7 @@ public class JDBCSessionDataStore extends AbstractSessionDataStore
                         data.setVhost(result.getString(_sessionTableSchema.getVirtualHostColumn())); //TODO needed??? this is part of the key now
 
                         try (InputStream is = _dbAdaptor.getBlobInputStream(result, _sessionTableSchema.getMapColumn());
-                                ClassLoadingObjectInputStream ois = new ClassLoadingObjectInputStream(is))
+                             ClassLoadingObjectInputStream ois = new ClassLoadingObjectInputStream(is))
                         {
                             Object o = ois.readObject();
                             data.putAllAttributes((Map<String,Object>)o);
diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/JDBCSessionIdManager.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/JDBCSessionIdManager.java
index 55d2a3df9d..534f2243f1 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/JDBCSessionIdManager.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/JDBCSessionIdManager.java
@@ -197,11 +197,11 @@ public class JDBCSessionIdManager extends org.eclipse.jetty.server.session.Abstr
      * @see org.eclipse.jetty.server.SessionIdManager#removeId(java.lang.String)
      */
     @Override
-    public void removeId (String id)
+    public boolean removeId (String id)
     {
 
         if (id == null)
-            return;
+            return false;
 
         synchronized (_sessionIds)
         {
@@ -210,11 +210,12 @@ public class JDBCSessionIdManager extends org.eclipse.jetty.server.session.Abstr
             try
             {
                 _sessionIds.remove(id);
-                delete(id);
+                return delete(id);
             }
             catch (Exception e)
             {
                 LOG.warn("Problem removing session id="+id, e);
+                return false;
             }
         }
 
@@ -255,7 +256,7 @@ public class JDBCSessionIdManager extends org.eclipse.jetty.server.session.Abstr
      * @param id
      * @throws SQLException
      */
-    private void delete (String id)
+    private boolean delete (String id)
     throws SQLException
     {
         try (Connection connection = _dbAdaptor.getConnection();
@@ -263,7 +264,7 @@ public class JDBCSessionIdManager extends org.eclipse.jetty.server.session.Abstr
         {
             connection.setAutoCommit(true);
             statement.setString(1, id);
-            statement.executeUpdate();
+            return (statement.executeUpdate() > 0);
         }
     }
     
diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/MemorySessionStore.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/MemorySessionStore.java
index e0e5a94f4b..9ed3505332 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/MemorySessionStore.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/MemorySessionStore.java
@@ -146,12 +146,12 @@ public class MemorySessionStore extends AbstractSessionStore
      * @see org.eclipse.jetty.server.session.AbstractSessionStore#doDelete(java.lang.String)
      */
     @Override
-    public boolean doDelete(String id)
+    public Session doDelete(String id)
     {
         Session s = _sessions.remove(id);
         if (s != null)
             _stats.decrement();
-        return  (s != null);
+        return  s;
     }
     
     
diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java
index 77e63611b6..06694bc5e2 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java
@@ -20,9 +20,11 @@
 package org.eclipse.jetty.server.session;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.Enumeration;
 import java.util.Iterator;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
 
 import javax.servlet.ServletContext;
 import javax.servlet.http.HttpServletRequest;
@@ -34,6 +36,8 @@ import javax.servlet.http.HttpSessionEvent;
 
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
+import org.eclipse.jetty.util.thread.Locker;
+import org.eclipse.jetty.util.thread.Locker.Lock;
 
 
 
@@ -50,13 +54,18 @@ public class Session implements SessionManager.SessionIf
     
     public final static String SESSION_CREATED_SECURE="org.eclipse.jetty.security.sessionCreatedSecure";
     
+    
+    public enum State {VALID, INVALID, INVALIDATING};
+        
+    
     protected SessionData _sessionData;
     protected SessionManager _manager;
     protected String _extendedId; //the _id plus the worker name
     protected long _requests;
     private boolean _idChanged;
     private boolean _newSession;
-    private boolean _doInvalidate; //remember we should invalidate this session
+    private State _state = State.VALID; //state of the session:valid,invalid or being invalidated
+    private Locker _lock = new Locker();
     
     public Session (HttpServletRequest request, SessionData data)
     {
@@ -87,7 +96,7 @@ public class Session implements SessionManager.SessionIf
     /* ------------------------------------------------------------- */
     protected void cookieSet()
     {
-        synchronized (this)
+        try (Lock lock = lock())
         {
            _sessionData.setCookieSet(_sessionData.getAccessed());
         }
@@ -95,7 +104,7 @@ public class Session implements SessionManager.SessionIf
     /* ------------------------------------------------------------ */
     protected boolean access(long time)
     {
-        synchronized(this)
+        try (Lock lock=lock())
         {
             if (!isValid())
                 return false;
@@ -118,30 +127,14 @@ public class Session implements SessionManager.SessionIf
     /* ------------------------------------------------------------ */
     protected void complete()
     {
-        synchronized(this)
+        try (Lock lock = lock())
         {
             _requests--;
-            if (_doInvalidate && _requests<=0  )
-                doInvalidate();
         }
     }
 
-
-    /* ------------------------------------------------------------- */
-    /** 
-     * 
-     * @throws Exception
-     */
-    protected void invalidateAndRemove() throws Exception
-    {
-        if (_manager == null)
-            throw new IllegalStateException ("No session manager for session "+ _sessionData.getId());
-        
-        _manager.removeSession(this,true);
-        doInvalidate();
-    }
-    
     
+
     /* ------------------------------------------------------------- */
     /** Check to see if session has expired as at the time given.
      * @param time the time in milliseconds
@@ -149,22 +142,35 @@ public class Session implements SessionManager.SessionIf
      */
     protected boolean isExpiredAt(long time)
     {
-        return _sessionData.isExpiredAt(time);
+        try (Lock lock = lock())
+        {
+            return _sessionData.isExpiredAt(time);
+        }
     }
     
-    
+    /* ------------------------------------------------------------ */
+    /**
+     * @param nodename
+     */
     public void setLastNode (String nodename)
     {
         _sessionData.setLastNode(nodename);
     }
     
-    
+    /* ------------------------------------------------------------ */
+    /**
+     * @return
+     */
     public String getLastNode ()
     {
         return _sessionData.getLastNode();
     }
 
-    public void clearAttributes()
+    /* ------------------------------------------------------------ */
+    /**
+     * 
+     */
+    protected void clearAttributes()
     {
         Set<String> keys = null;
 
@@ -173,13 +179,30 @@ public class Session implements SessionManager.SessionIf
             keys = _sessionData.getKeys();
             for (String key:keys)
             {
-                setAttribute(key,null);
+                clearAttribute(key);
             }
 
         }
         while (!keys.isEmpty());
     }
 
+    /* ------------------------------------------------------------ */
+    /** Unset the attribute.
+     * @param name
+     */
+    protected void clearAttribute (String name)
+    {
+        Object old=null;
+        try (Lock lock = lock())
+        {
+            //if session is not valid, don't accept the set
+
+            old=_sessionData.setAttribute(name,null);
+        }
+        if (old == null)
+            return; //if same as remove attribute but attribute was already removed, no change
+        callSessionAttributeListeners(name, null, old);
+    }
 
     /* ------------------------------------------------------------ */
     /**
@@ -231,73 +254,79 @@ public class Session implements SessionManager.SessionIf
         if (value!=null&&value instanceof HttpSessionBindingListener)
             ((HttpSessionBindingListener)value).valueBound(new HttpSessionBindingEvent(this,name));
     }
-    
+
 
     /* ------------------------------------------------------------- */
+    /**
+     * Call the activation listeners. This must be called holding the
+     * _lock.
+     */
     public void didActivate()
     {
-        synchronized(this)
+        HttpSessionEvent event = new HttpSessionEvent(this);
+        for (Iterator<String> iter = _sessionData.getKeys().iterator(); iter.hasNext();)
         {
-            HttpSessionEvent event = new HttpSessionEvent(this);
-            for (Iterator<String> iter = _sessionData.getKeys().iterator(); iter.hasNext();)
+            Object value = _sessionData.getAttribute(iter.next());
+            if (value instanceof HttpSessionActivationListener)
             {
-                Object value = _sessionData.getAttribute(iter.next());
-                if (value instanceof HttpSessionActivationListener)
-                {
-                    HttpSessionActivationListener listener = (HttpSessionActivationListener) value;
-                    listener.sessionDidActivate(event);
-                }
+                HttpSessionActivationListener listener = (HttpSessionActivationListener) value;
+                listener.sessionDidActivate(event);
             }
         }
     }
-    
-    
+
+
     /* ------------------------------------------------------------- */
+    /**
+     * Call the passivation listeners. This must be called holding the
+     * _lock
+     */
     public void willPassivate()
     {
-        synchronized(this)
+        HttpSessionEvent event = new HttpSessionEvent(this);
+        for (Iterator<String> iter = _sessionData.getKeys().iterator(); iter.hasNext();)
         {
-            HttpSessionEvent event = new HttpSessionEvent(this);
-            for (Iterator<String> iter = _sessionData.getKeys().iterator(); iter.hasNext();)
+            Object value = _sessionData.getAttribute(iter.next());
+            if (value instanceof HttpSessionActivationListener)
             {
-                Object value = _sessionData.getAttribute(iter.next());
-                if (value instanceof HttpSessionActivationListener)
-                {
-                    HttpSessionActivationListener listener = (HttpSessionActivationListener) value;
-                    listener.sessionWillPassivate(event);
-                }
+                HttpSessionActivationListener listener = (HttpSessionActivationListener) value;
+                listener.sessionWillPassivate(event);
             }
         }
     }
-    
+
     /* ------------------------------------------------------------ */
     public boolean isValid()
     {
-        return !_sessionData.isInvalid();
+        try (Lock lock = lock())
+        {
+            return _state==State.VALID;
+        }
     }
 
 
     /* ------------------------------------------------------------- */
     public long getCookieSetTime()
     {
-        return _sessionData.getCookieSet();
-    }
-    
-    /* ------------------------------------------------------------- */
-    public void setCookieSetTime(long time)
-    {
-        _sessionData.setCookieSet(time);
+        try (Lock lock = lock())
+        {
+            return _sessionData.getCookieSet();
+        }
     }
 
+
     /* ------------------------------------------------------------- */
     @Override
     public long getCreationTime() throws IllegalStateException
     {
-        checkValid();
-        return _sessionData.getCreated();
+        try (Lock lock = lock())
+        {
+            checkValid();
+            return _sessionData.getCreated();
+        }
     }
-    
-   
+
+
 
     /** 
      * @see javax.servlet.http.HttpSession#getId()
@@ -305,9 +334,12 @@ public class Session implements SessionManager.SessionIf
     @Override
     public String getId()
     {
-        return _sessionData.getId();
+        try (Lock lock = lock())
+        {
+            return _sessionData.getId();
+        }
     }
-    
+
     
     public String getExtendedId()
     {
@@ -325,14 +357,17 @@ public class Session implements SessionManager.SessionIf
         return _sessionData.getVhost();
     }
     
-    
+
     /** 
      * @see javax.servlet.http.HttpSession#getLastAccessedTime()
      */
     @Override
     public long getLastAccessedTime()
     {
-        return _sessionData.getLastAccessed();
+        try (Lock lock = lock())
+        {
+            return _sessionData.getLastAccessed();
+        }
     }
 
     /** 
@@ -352,9 +387,12 @@ public class Session implements SessionManager.SessionIf
     @Override
     public void setMaxInactiveInterval(int secs)
     {
-       _sessionData.setMaxInactiveMs((long)secs*1000L);  
-       _sessionData.setExpiry(_sessionData.getMaxInactiveMs() <= 0 ? 0 : (System.currentTimeMillis() + _sessionData.getMaxInactiveMs()*1000L));
-       _sessionData.setDirty(true);
+        try (Lock lock = lock())
+        {
+            _sessionData.setMaxInactiveMs((long)secs*1000L);  
+            _sessionData.setExpiry(_sessionData.getMaxInactiveMs() <= 0 ? 0 : (System.currentTimeMillis() + _sessionData.getMaxInactiveMs()*1000L));
+            _sessionData.setDirty(true);
+        }
     }
 
     /** 
@@ -363,7 +401,10 @@ public class Session implements SessionManager.SessionIf
     @Override
     public int getMaxInactiveInterval()
     {
-        return (int)(_sessionData.getMaxInactiveMs()/1000);
+        try (Lock lock = lock())
+        {
+            return (int)(_sessionData.getMaxInactiveMs()/1000);
+        }
     }
 
     /** 
@@ -386,24 +427,29 @@ public class Session implements SessionManager.SessionIf
     /* ------------------------------------------------------------- */
     /**
      * asserts that the session is valid
-     * @throws IllegalStateException if the sesion is invalid
+     * @throws IllegalStateException if the session is invalid
      */
     protected void checkValid() throws IllegalStateException
-    {
-        if (_sessionData.isInvalid())
+    {    
+        if (!_lock.isLocked())
+            throw new IllegalStateException();
+
+        if (_state != State.VALID)
             throw new IllegalStateException();
     }
-    
-    
+
+
     /** 
      * @see javax.servlet.http.HttpSession#getAttribute(java.lang.String)
      */
     @Override
     public Object getAttribute(String name)
     {
-        //TODO synchronization
-        checkValid();
-        return _sessionData.getAttribute(name);
+        try (Lock lock = lock())
+        {
+            checkValid();
+            return _sessionData.getAttribute(name);
+        }
     }
 
     /** 
@@ -412,8 +458,10 @@ public class Session implements SessionManager.SessionIf
     @Override
     public Object getValue(String name)
     {
-        // TODO synchronization
-        return _sessionData.getAttribute(name);
+        try (Lock lock = lock())
+        { 
+            return _sessionData.getAttribute(name);
+        }
     }
 
     /** 
@@ -422,7 +470,7 @@ public class Session implements SessionManager.SessionIf
     @Override
     public Enumeration<String> getAttributeNames()
     {
-        synchronized (this)
+        try (Lock lock = lock())
         {
             checkValid();
             final Iterator<String> itor = _sessionData.getKeys().iterator();
@@ -446,24 +494,24 @@ public class Session implements SessionManager.SessionIf
     }
 
 
-    
+
 
     /* ------------------------------------------------------------ */
     public int getAttributes()
     {
         return _sessionData.getKeys().size();
     }
-  
 
- 
+
+
 
     /* ------------------------------------------------------------ */
     public Set<String> getNames()
     {
-        return _sessionData.getKeys();
+        return Collections.unmodifiableSet(_sessionData.getKeys());
     }
-  
-    
+
+
     /* ------------------------------------------------------------- */
     /**
      * @deprecated As of Version 2.2, this method is replaced by
@@ -473,7 +521,7 @@ public class Session implements SessionManager.SessionIf
     @Override
     public String[] getValueNames() throws IllegalStateException
     {
-        synchronized(this)
+        try (Lock lock = lock())
         {
             checkValid();
             Iterator<String> itor = _sessionData.getKeys().iterator();
@@ -486,7 +534,7 @@ public class Session implements SessionManager.SessionIf
         }
     }
 
-
+    /* ------------------------------------------------------------- */
     /** 
      * @see javax.servlet.http.HttpSession#setAttribute(java.lang.String, java.lang.Object)
      */
@@ -494,7 +542,7 @@ public class Session implements SessionManager.SessionIf
     public void setAttribute(String name, Object value)
     {
         Object old=null;
-        synchronized (this)
+        try (Lock lock = lock())
         {
             //if session is not valid, don't accept the set
             checkValid();
@@ -504,7 +552,10 @@ public class Session implements SessionManager.SessionIf
             return; //if same as remove attribute but attribute was already removed, no change
         callSessionAttributeListeners(name, value, old);
     }
-
+    
+    
+    
+    /* ------------------------------------------------------------- */
     /** 
      * @see javax.servlet.http.HttpSession#putValue(java.lang.String, java.lang.Object)
      */
@@ -513,7 +564,10 @@ public class Session implements SessionManager.SessionIf
     {
         setAttribute(name,value);
     }
-
+    
+    
+    
+    /* ------------------------------------------------------------- */
     /** 
      * @see javax.servlet.http.HttpSession#removeAttribute(java.lang.String)
      */
@@ -522,7 +576,10 @@ public class Session implements SessionManager.SessionIf
     {
        setAttribute(name, null);
     }
-
+    
+    
+    
+    /* ------------------------------------------------------------- */
     /** 
      * @see javax.servlet.http.HttpSession#removeValue(java.lang.String)
      */
@@ -538,7 +595,21 @@ public class Session implements SessionManager.SessionIf
         if (_manager == null)
             throw new IllegalStateException ("No session manager for session "+ _sessionData.getId());
         
-        _manager._sessionIdManager.renewSessionId(_sessionData.getId(), getExtendedId(), request); 
+        //TODO
+        //simultaneous calls to renew id on same session - all subsequent calls to renew need to wait 
+        //until first renew finishes - and then should they proceed in order, or does first renew win????
+        //Or are simultaneous interleaved renewals ok, so long as we end up with a consistent result???
+        
+        String id = null;
+        String extendedId = null;
+        try (Lock lock = lock())
+        {
+            checkValid(); //don't renew id on a session that is not valid
+            id = _sessionData.getId(); //grab the values as they are now
+            extendedId = getExtendedId();
+        }
+        
+        _manager._sessionIdManager.renewSessionId(id, extendedId, request); 
         setIdChanged(true);
     }
        
@@ -546,7 +617,8 @@ public class Session implements SessionManager.SessionIf
     /* ------------------------------------------------------------- */
     /** Called by users to invalidate a session, or called by the
      * access method as a request enters the session if the session
-     * has expired.
+     * has expired, or called by manager as a result of scavenger
+     * expiring session
      * 
      * @see javax.servlet.http.HttpSession#invalidate()
      */
@@ -555,59 +627,115 @@ public class Session implements SessionManager.SessionIf
     {
         if (_manager == null)
             throw new IllegalStateException ("No session manager for session "+ _sessionData.getId());
-        
-        checkValid();
+
+        boolean result = false;
+
+        try (Lock lock = lock())
+        {
+            switch (_state)
+            {
+                case INVALID:
+                {
+                    throw new IllegalStateException(); //spec does not allow invalidate of already invalid session
+                }
+                case VALID:
+                {
+                    //only first change from valid to invalidating should be actionable
+                    result = true;
+                    _state = State.INVALIDATING;
+                    break;
+                }
+                default:
+                {
+                    LOG.info("Session {} already being invalidated", _sessionData.getId());
+                }
+            }
+        }
+
         try
         {
-            //tell id mgr to remove session from all other contexts
-           ((AbstractSessionIdManager)_manager.getSessionIdManager()).invalidateAll(_sessionData.getId());
-           
+            //if the session was not already invalid, or in process of being invalidated, do invalidate
+            if (result)
+            {
+                //tell id mgr to remove session from all other contexts
+                ((AbstractSessionIdManager)_manager.getSessionIdManager()).invalidateAll(_sessionData.getId());
+            }
         }
         catch (Exception e)
         {
             LOG.warn(e);
         }
     }
+    
+    
+    
+    /* ------------------------------------------------------------- */
+    /** Grab the lock on the session
+     * @return
+     */
+    public Lock lock ()
+    {
+        return _lock.lock();
+    }
+
+
+
+
 
     /* ------------------------------------------------------------- */
     protected void doInvalidate() throws IllegalStateException
     {
-        try
+        try (Lock lock = lock())
         {
-            if (LOG.isDebugEnabled())
-                LOG.debug("invalidate {}",_sessionData.getId());
-            if (isValid())
-                clearAttributes();
-        }
-        finally
-        {
-            synchronized (this)
+            try
+            {
+                if (LOG.isDebugEnabled())
+                    LOG.debug("invalidate {}",_sessionData.getId());
+                if (isValid())
+                    clearAttributes();
+            }
+            finally
             {
                 // mark as invalid
-                _sessionData.setInvalid(true);
+                _state = State.INVALID;
             }
         }
     }
-   
+
     /* ------------------------------------------------------------- */
     @Override
     public boolean isNew() throws IllegalStateException
     {
-        checkValid();
-        return _newSession;
+        try (Lock lock = lock())
+        {
+            checkValid();
+            return _newSession;
+        }
     }
     
+    
+
     /* ------------------------------------------------------------- */
     public void setIdChanged(boolean changed)
     {
-        _idChanged=changed;
+        try (Lock lock = lock())
+        {
+            _idChanged=changed;
+        }
     }
     
+    
+    /* ------------------------------------------------------------- */
     public boolean isIdChanged ()
     {
-        return _idChanged;
+        try (Lock lock = lock())
+        {
+            return _idChanged;
+        }
     }
     
+    
+    /* ------------------------------------------------------------- */
     /** 
      * @see org.eclipse.jetty.server.session.AbstractSessionManager.SessionIf#getSession()
      */
@@ -618,7 +746,7 @@ public class Session implements SessionManager.SessionIf
         return this;
     }
   
-    
+    /* ------------------------------------------------------------- */
     protected SessionData getSessionData()
     {
         return _sessionData;
diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionData.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionData.java
index 761caa496b..1cebc42be7 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionData.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionData.java
@@ -38,6 +38,7 @@ import java.util.concurrent.ConcurrentHashMap;
 public class SessionData implements Serializable
 {
 
+   
     private static final long serialVersionUID = 1L;
 
     protected String _id;
@@ -53,7 +54,7 @@ public class SessionData implements Serializable
     protected long _cookieSet;
     protected long _accessed;         // the time of the last access
     protected long _lastAccessed;     // the time of the last access excluding this one
-    protected boolean _invalid;
+   // protected boolean _invalid;
     protected long _maxInactiveMs;
     protected Map<String,Object> _attributes = new ConcurrentHashMap<String, Object>();
     protected boolean _dirty;
@@ -232,15 +233,12 @@ public class SessionData implements Serializable
         _lastAccessed = lastAccessed;
     }
 
-    public boolean isInvalid()
+   /* public boolean isInvalid()
     {
         return _invalid;
     }
-
-    public void setInvalid(boolean invalid)
-    {
-        _invalid = invalid;
-    }
+*/
+ 
 
     public long getMaxInactiveMs()
     {
diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionManager.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionManager.java
index 9fe1125346..3721c76725 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionManager.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionManager.java
@@ -55,7 +55,7 @@ import org.eclipse.jetty.util.statistic.CounterStatistic;
 import org.eclipse.jetty.util.statistic.SampleStatistic;
 
 /**
- * AbstractSessionManager
+ * SessionManager
  *
  *
  */
@@ -748,7 +748,10 @@ public class SessionManager extends ContainerLifeCycle implements org.eclipse.je
     }
 
 
-    
+    /* ------------------------------------------------------------ */
+    /**
+     * @return
+     */
     public SessionStore getSessionStore ()
     {
         return _sessionStore;
@@ -776,18 +779,18 @@ public class SessionManager extends ContainerLifeCycle implements org.eclipse.je
     /* ------------------------------------------------------------ */
     /** 
      * Remove session from manager
-     * @param session The session to remove
+     * @param id The session to remove
      * @param invalidate True if {@link HttpSessionListener#sessionDestroyed(HttpSessionEvent)} and
      * {@link SessionIdManager#expireAll(String)} should be called.
      * @return if the session was removed 
      */
-    public boolean removeSession(Session session, boolean invalidate)
+    public Session removeSession(String id, boolean invalidate)
     {
         try
         {
             //Remove the Session object from the session store and any backing data store
-            boolean removed = _sessionStore.delete(session.getId());
-            if (removed)
+            Session session = _sessionStore.delete(id);
+            if (session != null)
             {
                 if (invalidate)
                 {
@@ -802,12 +805,12 @@ public class SessionManager extends ContainerLifeCycle implements org.eclipse.je
                 }
             }
 
-            return removed;
+            return session;
         }
         catch (Exception e)
         {
             LOG.warn(e);
-            return false;
+            return null;
         }
     }
     
@@ -939,11 +942,9 @@ public class SessionManager extends ContainerLifeCycle implements org.eclipse.je
             session.getSessionData().setId(newId);
             session.setExtendedId(newExtendedId);
             session.getSessionData().setLastSaved(0); //forces an insert
-            session.getSessionData().setDirty(true);
+            session.getSessionData().setDirty(true);  //forces an insert
             _sessionStore.put(newId, session);
-            
-            Session x = _sessionStore.get(newId, false);
-            
+  
             //tell session id manager the id is in use
             _sessionIdManager.useId(session);
             
@@ -968,7 +969,13 @@ public class SessionManager extends ContainerLifeCycle implements org.eclipse.je
     
    
     
-    
+    /* ------------------------------------------------------------ */
+    /**
+     * Called either when a session has expired, or the app has
+     * invalidated it.
+     * 
+     * @param id
+     */
     public void invalidate (String id)
     {
         if (StringUtil.isBlank(id))
@@ -976,23 +983,26 @@ public class SessionManager extends ContainerLifeCycle implements org.eclipse.je
 
         try
         {
-            Session session = _sessionStore.get(id, false);
-            if (session == null)
+            //remove the session and call the destroy listeners
+            Session session = removeSession(id, true);
+
+            if (session != null)
             {
-                return;  // couldn't get/load a session for this context with that id
+                _sessionTimeStats.set(round((System.currentTimeMillis() - session.getSessionData().getCreated())/1000.0));
+                session.doInvalidate();   
             }
-            
-            _sessionTimeStats.set(round((System.currentTimeMillis() - session.getSessionData().getCreated())/1000.0));
-            session.invalidateAndRemove();           
         }
         catch (Exception e)
         {
             LOG.warn(e);
         }
     }
+
     
-    
-    
+    /* ------------------------------------------------------------ */
+    /**
+     * @return
+     */
     public Set<String> scavenge ()
     {
         //don't attempt to scavenge if we are shutting down
diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionStore.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionStore.java
index 31c72400be..a725192980 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionStore.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionStore.java
@@ -41,7 +41,7 @@ public interface SessionStore extends LifeCycle
     Session get(String id, boolean staleCheck) throws Exception;
     void put(String id, Session session) throws Exception;
     boolean exists (String id) throws Exception;
-    boolean delete (String id) throws Exception;
+    Session delete (String id) throws Exception;
     void shutdown ();
     Set<String> getExpired ();
 }
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/session/SessionCookieTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/session/SessionCookieTest.java
index b7e7582768..2869657b59 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/session/SessionCookieTest.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/session/SessionCookieTest.java
@@ -107,9 +107,9 @@ public class SessionCookieTest
          * @see org.eclipse.jetty.server.session.AbstractSessionStore#doDelete(org.eclipse.jetty.server.session.SessionKey)
          */
         @Override
-        public boolean doDelete(String key)
+        public Session doDelete(String key)
         {
-            return false;
+            return null;
         }
 
         /** 
@@ -175,10 +175,9 @@ public class SessionCookieTest
          * @see org.eclipse.jetty.server.SessionIdManager#removeId(java.lang.String)
          */
         @Override
-        public void removeId(String id)
+        public boolean removeId(String id)
         {
-            // TODO Auto-generated method stub
-            
+            return true;
         }
     }