Skip to content

Commit

Permalink
Issue #4156 Remove use of PlaceHolderSession for simultaneous session…
Browse files Browse the repository at this point in the history 
… loading (#4304)

* Issue #4156 Remove use of PlaceHolderSession for simultaneous session loading.
  • Loading branch information
@janbartel
janbartel committed Nov 17, 2019
1 parent e69eea8 commit 318246e
Show file tree
Hide file tree
Showing 6 changed files with 84 additions and 227 deletions.
56 changes: 2 additions & 54 deletions ...hed-sessions/src/test/java/org/eclipse/jetty/memcached/session/TestMemcachedSessions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import java.io.IOException;
import java.io.PrintWriter;

import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
Expand All @@ -31,12 +32,9 @@
import org.eclipse.jetty.client.api.Request;
import org.eclipse.jetty.server.NetworkConnector;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.session.AbstractSessionCache;
import org.eclipse.jetty.server.session.CachingSessionDataStore;
import org.eclipse.jetty.server.session.NullSessionCache;
import org.eclipse.jetty.server.session.NullSessionDataStore;
import org.eclipse.jetty.server.session.Session;
import org.eclipse.jetty.server.session.SessionData;
import org.eclipse.jetty.server.session.SessionHandler;
import org.eclipse.jetty.servlet.ServletContextHandler;
import org.eclipse.jetty.servlet.ServletHolder;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -88,56 +86,6 @@ else if ("del".equals(arg))
}
}

public static class NullSessionCache extends AbstractSessionCache
{

public NullSessionCache(SessionHandler handler)
{
super(handler);
}

@Override
public void shutdown()
{
}

@Override
public Session newSession(SessionData data)
{
return new Session(_handler, data);
}

@Override
public Session newSession(HttpServletRequest request, SessionData data)
{
return new Session(_handler, request, data);
}

@Override
public Session doGet(String id)
{
return null;
}

@Override
public Session doPutIfAbsent(String id, Session session)
{
return null;
}

@Override
public boolean doReplace(String id, Session oldValue, Session newValue)
{
return true;
}

@Override
public Session doDelete(String id)
{
return null;
}
}

@Test
public void testMemcached() throws Exception
{
Expand Down
148 changes: 43 additions & 105 deletions jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import java.util.Collections;
import java.util.HashSet;
import java.util.Set;
import java.util.function.Function;

import javax.servlet.http.HttpServletRequest;

import org.eclipse.jetty.util.StringUtil;
Expand Down Expand Up @@ -133,6 +135,18 @@ public abstract class AbstractSessionCache extends ContainerLifeCycle implements
* @return null if the session wasn't already in the map, or the existing entry otherwise
*/
protected abstract Session doPutIfAbsent(String id, Session session);

/**
* Compute the mappingFunction to create a Session object iff the session
* with the given id isn't already in the map, otherwise return the existing Session.
* This method is expected to have precisely the same behaviour as
* {@link java.util.concurrent.ConcurrentHashMap#computeIfAbsent}
*
* @param id the session id
* @param mappingFunction the function to load the data for the session
* @return an existing Session from the cache
*/
protected abstract Session doComputeIfAbsent(String id, Function<String, Session> mappingFunction);

/**
* Replace the mapping from id to oldValue with newValue
Expand All @@ -152,22 +166,6 @@ public abstract class AbstractSessionCache extends ContainerLifeCycle implements
*/
public abstract Session doDelete(String id);

/**
* PlaceHolder
*/
protected class PlaceHolderSession extends Session
{

/**
* @param handler SessionHandler to which this session belongs
* @param data the session data
*/
public PlaceHolderSession(SessionHandler handler, SessionData data)
{
super(handler, data);
}
}

/**
* @param handler the {@link SessionHandler} to use
*/
Expand Down Expand Up @@ -343,113 +341,53 @@ public Session get(String id) throws Exception
protected Session getAndEnter(String id, boolean enter) throws Exception
{
Session session = null;
Exception ex = null;

while (true)
session = doComputeIfAbsent(id, k ->
{
session = doGet(id);

if (_sessionDataStore == null)
break; //can't load any session data so just return null or the session object
if (LOG.isDebugEnabled())
LOG.debug("Session {} not found locally in {}, attempting to load", id, this);

if (session == null)
try
{
if (LOG.isDebugEnabled())
LOG.debug("Session {} not found locally in {}, attempting to load", id, this);

//didn't get a session, try and create one and put in a placeholder for it
PlaceHolderSession phs = new PlaceHolderSession(_handler, new SessionData(id, null, null, 0, 0, 0, 0));
Lock phsLock = phs.lock();
Session s = doPutIfAbsent(id, phs);
if (s == null)
Session s = loadSession(k);
if (s != null)
{
//My placeholder won, go ahead and load the full session data
try
{
session = loadSession(id);
if (session == null)
{
//session does not exist, remove the placeholder
doDelete(id);
phsLock.close();
break;
}

try (Lock lock = session.lock())
{
//swap it in instead of the placeholder
boolean success = doReplace(id, phs, session);
if (!success)
{
//something has gone wrong, it should have been our placeholder
doDelete(id);
session = null;
LOG.warn("Replacement of placeholder for session {} failed", id);
phsLock.close();
break;
}
else
{
//successfully swapped in the session
session.setResident(true);
if (enter)
session.use();
phsLock.close();
break;
}
}
}
catch (Exception e)
try (Lock lock = s.lock())
{
ex = e; //remember a problem happened loading the session
doDelete(id); //remove the placeholder
phsLock.close();
session = null;
break;
s.setResident(true); //ensure freshly loaded session is resident
}
}
else
{
//my placeholder didn't win, check the session returned
phsLock.close();
try (Lock lock = s.lock())
{
//is it a placeholder? or is a non-resident session? In both cases, chuck it away and start again
if (!s.isResident() || s instanceof PlaceHolderSession)
{
session = null;
continue;
}
//I will use this session too
session = s;
if (enter)
session.use();
break;
}
if (LOG.isDebugEnabled())
LOG.debug("Session {} not loaded by store", id);
}
return s;
}
else
catch (Exception e)
{
//check the session returned
try (Lock lock = session.lock())
{
//is it a placeholder? or is it passivated? In both cases, chuck it away and start again
if (!session.isResident() || session instanceof PlaceHolderSession)
{
session = null;
continue;
}
LOG.warn("Error loading session {}", id, e);
return null;
}
});

//got the session
if (enter)
session.use();
break;
if (session != null)
{
try (Lock lock = session.lock())
{
if (!session.isResident()) //session isn't marked as resident in cache
{
if (LOG.isDebugEnabled())
LOG.debug("Non-resident session {} in cache", id);
return null;
}
else if (enter)
{
session.use();
}
}
}

if (ex != null)
throw ex;
return session;
}

Expand Down
41 changes: 16 additions & 25 deletions jetty-server/src/main/java/org/eclipse/jetty/server/session/DefaultSessionCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
package org.eclipse.jetty.server.session;

import java.util.concurrent.ConcurrentHashMap;
import java.util.function.Function;

import javax.servlet.http.HttpServletRequest;

import org.eclipse.jetty.util.annotation.ManagedAttribute;
Expand Down Expand Up @@ -80,18 +82,12 @@ public long getSessionsTotal()
return _stats.getTotal();
}

/**
*
*/
@ManagedOperation(value = "reset statistics", impact = "ACTION")
public void resetStats()
{
_stats.reset();
}

/**
* @see org.eclipse.jetty.server.session.AbstractSessionCache#doGet(java.lang.String)
*/
@Override
public Session doGet(String id)
{
Expand All @@ -103,26 +99,32 @@ public Session doGet(String id)
return session;
}

/**
* @see org.eclipse.jetty.server.session.AbstractSessionCache#doPutIfAbsent(java.lang.String, org.eclipse.jetty.server.session.Session)
*/
@Override
public Session doPutIfAbsent(String id, Session session)
{
Session s = _sessions.putIfAbsent(id, session);
if (s == null && !(session instanceof PlaceHolderSession))
if (s == null)
_stats.increment();
return s;
}

/**
* @see org.eclipse.jetty.server.session.AbstractSessionCache#doDelete(java.lang.String)
*/
@Override
protected Session doComputeIfAbsent(String id, Function<String, Session> mappingFunction)
{
return _sessions.computeIfAbsent(id, k ->
{
Session s = mappingFunction.apply(k);
if (s != null)
_stats.increment();
return s;
});
}

@Override
public Session doDelete(String id)
{
Session s = _sessions.remove(id);
if (s != null && !(s instanceof PlaceHolderSession))
if (s != null)
_stats.decrement();
return s;
}
Expand Down Expand Up @@ -168,35 +170,24 @@ public void shutdown()
}
}

/**
* @see org.eclipse.jetty.server.session.AbstractSessionCache#newSession(javax.servlet.http.HttpServletRequest, org.eclipse.jetty.server.session.SessionData)
*/
@Override
public Session newSession(HttpServletRequest request, SessionData data)
{
Session s = new Session(getSessionHandler(), request, data);
return s;
}

/**
* @see org.eclipse.jetty.server.session.AbstractSessionCache#newSession(org.eclipse.jetty.server.session.SessionData)
*/
@Override
public Session newSession(SessionData data)
{
Session s = new Session(getSessionHandler(), data);
return s;
}

/**
* @see org.eclipse.jetty.server.session.AbstractSessionCache#doReplace(java.lang.String, org.eclipse.jetty.server.session.Session, org.eclipse.jetty.server.session.Session)
*/
@Override
public boolean doReplace(String id, Session oldValue, Session newValue)
{
boolean result = _sessions.replace(id, oldValue, newValue);
if (result && (oldValue instanceof PlaceHolderSession))
_stats.increment();
return result;
}
}

0 comments on commit 318246e

Please sign in to comment.