Skip to content

Commit

Permalink
Issue #6277 Better handling of exceptions thrown in sessionDestroyed (#…
Browse files Browse the repository at this point in the history 
…6278) (#6279)

* Issue #6277 Better handling of exceptions thrown in sessionDestroyed

Signed-off-by: Jan Bartel <[email protected]>
  • Loading branch information
@janbartel
janbartel authored May 16, 2021
1 parent edcaf70 commit 087f486
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 19 deletions.
13 changes: 7 additions & 6 deletions jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -498,10 +498,7 @@ public long getLastAccessedTime()
{
try (Lock lock = _lock.lock())
{
if (isInvalid())
{
throw new IllegalStateException("Session not valid");
}
checkValidForRead();
return _sessionData.getLastAccessed();
}
}
Expand Down Expand Up @@ -947,14 +944,18 @@ public void invalidate()
// do the invalidation
_handler.callSessionDestroyedListeners(this);
}
catch (Exception e)
{
LOG.warn("Error during Session destroy listener", e);
}
finally
{
// call the attribute removed listeners and finally mark it
// as invalid
finishInvalidate();
// tell id mgr to remove sessions with same id from all contexts
_handler.getSessionIdManager().invalidateAll(_sessionData.getId());
}
// tell id mgr to remove sessions with same id from all contexts
_handler.getSessionIdManager().invalidateAll(_sessionData.getId());
}
}
catch (Exception e)
Expand Down
24 changes: 19 additions & 5 deletions ...ssions-common/src/main/java/org/eclipse/jetty/server/session/TestHttpSessionListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,18 @@ public class TestHttpSessionListener implements HttpSessionListener
public List<String> createdSessions = new ArrayList<>();
public List<String> destroyedSessions = new ArrayList<>();
public boolean accessAttribute = false;
public Exception ex = null;
public boolean lastAccessTime = false;
public Exception attributeException = null;
public Exception accessTimeException = null;

public TestHttpSessionListener(boolean access)
public TestHttpSessionListener(boolean accessAttribute, boolean lastAccessTime)
{
accessAttribute = access;
this.accessAttribute = accessAttribute;
this.lastAccessTime = lastAccessTime;
}

public TestHttpSessionListener()
{
accessAttribute = false;
}

public void sessionDestroyed(HttpSessionEvent se)
Expand All @@ -54,7 +56,19 @@ public void sessionDestroyed(HttpSessionEvent se)
}
catch (Exception e)
{
ex = e;
attributeException = e;
}
}

if (lastAccessTime)
{
try
{
se.getSession().getLastAccessedTime();
}
catch (Exception e)
{
accessTimeException = e;
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions .../main/java/org/eclipse/jetty/server/session/TestHttpSessionListenerWithWebappClasses.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ public TestHttpSessionListenerWithWebappClasses()
super();
}

public TestHttpSessionListenerWithWebappClasses(boolean access)
public TestHttpSessionListenerWithWebappClasses(boolean attribute, boolean lastAccessTime)
{
super(access);
super(attribute, lastAccessTime);
}

@Override
Expand All @@ -52,7 +52,7 @@ public void sessionDestroyed(HttpSessionEvent se)
}
catch (Exception cnfe)
{
ex = cnfe;
attributeException = cnfe;
}
super.sessionDestroyed(se);
}
Expand Down
95 changes: 90 additions & 5 deletions ...t-sessions-common/src/test/java/org/eclipse/jetty/server/session/SessionListenerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
import static org.hamcrest.Matchers.in;
import static org.hamcrest.Matchers.is;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
Expand Down Expand Up @@ -92,7 +93,7 @@ public void testListenerWithInvalidation() throws Exception
TestServer server = new TestServer(0, inactivePeriod, scavengePeriod,
cacheFactory, storeFactory);
ServletContextHandler context = server.addContext(contextPath);
TestHttpSessionListener listener = new TestHttpSessionListener(true);
TestHttpSessionListener listener = new TestHttpSessionListener(true, true);
context.getSessionHandler().addEventListener(listener);
TestServlet servlet = new TestServlet();
ServletHolder holder = new ServletHolder(servlet);
Expand Down Expand Up @@ -136,6 +137,72 @@ public void testListenerWithInvalidation() throws Exception
LifeCycle.stop(server);
}
}

/**
* Test that if a session listener throws an exception during sessionDestroyed the session is still invalidated
*/
@Test
public void testListenerWithInvalidationException() throws Exception
{
String contextPath = "";
String servletMapping = "/server";
int inactivePeriod = 6;
int scavengePeriod = -1;

DefaultSessionCacheFactory cacheFactory = new DefaultSessionCacheFactory();
cacheFactory.setEvictionPolicy(SessionCache.NEVER_EVICT);
TestSessionDataStoreFactory storeFactory = new TestSessionDataStoreFactory();
storeFactory.setGracePeriodSec(scavengePeriod);

TestServer server = new TestServer(0, inactivePeriod, scavengePeriod,
cacheFactory, storeFactory);
ServletContextHandler context = server.addContext(contextPath);
ThrowingSessionListener listener = new ThrowingSessionListener();
context.getSessionHandler().addEventListener(listener);
TestServlet servlet = new TestServlet();
ServletHolder holder = new ServletHolder(servlet);
context.addServlet(holder, servletMapping);

try
{
server.start();
int port1 = server.getPort();

HttpClient client = new HttpClient();
client.start();
try
{
String url = "http://localhost:" + port1 + contextPath + servletMapping;
// Create the session
ContentResponse response1 = client.GET(url + "?action=init");
assertEquals(HttpServletResponse.SC_OK, response1.getStatus());
String sessionCookie = response1.getHeaders().get("Set-Cookie");
assertNotNull(sessionCookie);
assertTrue(TestServlet.bindingListener.bound);

String sessionId = TestServer.extractSessionId(sessionCookie);

// Make a request which will invalidate the existing session
Request request2 = client.newRequest(url + "?action=test");
ContentResponse response2 = request2.send();
assertEquals(HttpServletResponse.SC_OK, response2.getStatus());

assertTrue(TestServlet.bindingListener.unbound);

//check session no longer exists
assertFalse(context.getSessionHandler().getSessionCache().contains(sessionId));
assertFalse(context.getSessionHandler().getSessionCache().getSessionDataStore().exists(sessionId));
}
finally
{
LifeCycle.stop(client);
}
}
finally
{
LifeCycle.stop(server);
}
}

/**
* Test that listeners are called when a session expires
Expand Down Expand Up @@ -177,7 +244,7 @@ public void testSessionExpiresWithListener() throws Exception
ServletContextHandler context = server1.addContext(contextPath);
context.setClassLoader(contextClassLoader);
context.addServlet(holder, servletMapping);
TestHttpSessionListener listener = new TestHttpSessionListenerWithWebappClasses(true);
TestHttpSessionListener listener = new TestHttpSessionListenerWithWebappClasses(true, true);
context.getSessionHandler().addEventListener(listener);

try
Expand Down Expand Up @@ -206,7 +273,8 @@ public void testSessionExpiresWithListener() throws Exception

assertThat(sessionId, is(in(listener.destroyedSessions)));

assertNull(listener.ex);
assertNull(listener.attributeException);
assertNull(listener.accessTimeException);
}
finally
{
Expand Down Expand Up @@ -241,7 +309,7 @@ public void testExpiredSession() throws Exception
ServletHolder holder = new ServletHolder(servlet);
ServletContextHandler context = server1.addContext(contextPath);
context.addServlet(holder, servletMapping);
TestHttpSessionListener listener = new TestHttpSessionListener();
TestHttpSessionListener listener = new TestHttpSessionListener(true, true);

context.getSessionHandler().addEventListener(listener);

Expand Down Expand Up @@ -276,7 +344,8 @@ public void testExpiredSession() throws Exception

assertTrue(listener.destroyedSessions.contains("1234"));

assertNull(listener.ex);
assertNull(listener.attributeException);
assertNull(listener.accessTimeException);
}
finally
{
Expand All @@ -301,6 +370,22 @@ public void sessionDestroyed(HttpSessionEvent se)
{
}
}

public static class ThrowingSessionListener implements HttpSessionListener
{

@Override
public void sessionCreated(HttpSessionEvent se)
{
}

@Override
public void sessionDestroyed(HttpSessionEvent se)
{
throw new IllegalStateException("Exception during sessionDestroyed");
}

}

@Test
public void testSessionListeners()
Expand Down

0 comments on commit 087f486

Please sign in to comment.