Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue #6277 Better handling of exceptions thrown in sessionDestroyed #6278

Merged
merged 2 commits into from May 15, 2021
Merged

Issue #6277 Better handling of exceptions thrown in sessionDestroyed #6278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b926efb
Issue #6277 Better handling of exceptions thrown in sessionDestroyed
janbartel May 14, 2021
5d95ad5
Issue #6277 Fix RequestTest.testPushBuilder
janbartel May 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 7 additions & 6 deletions jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java
View file
Open in desktop
Expand Up @@ -469,10 +469,7 @@ public long getLastAccessedTime()
{
try (AutoLock l = _lock.lock())
{
if (isInvalid())
{
throw new IllegalStateException("Session not valid");
}
checkValidForRead();
return _sessionData.getLastAccessed();
}
}
Expand Down Expand Up @@ -867,14 +864,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
4 changes: 3 additions & 1 deletion jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java
View file
Open in desktop
Expand Up @@ -1972,7 +1972,9 @@ public boolean isPushSupported()
@Override
public HttpSession getSession()
{
return new Session(new SessionHandler(), new SessionData(TEST_SESSION_ID, "", "0.0.0.0", 0, 0, 0, 300));
Session session = new Session(new SessionHandler(), new SessionData(TEST_SESSION_ID, "", "0.0.0.0", 0, 0, 0, 300));
session.setResident(true); //necessary for session methods to not throw ISE
return session;
}

@Override
Expand Down
24 changes: 19 additions & 5 deletions ...ssions-common/src/main/java/org/eclipse/jetty/server/session/TestHttpSessionListener.java
View file
Open in desktop
Expand Up @@ -26,16 +26,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 @@ -49,7 +51,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
Open in desktop
Expand Up @@ -30,9 +30,9 @@ public TestHttpSessionListenerWithWebappClasses()
super();
}

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

@Override
Expand All @@ -47,7 +47,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
Open in desktop
Expand Up @@ -53,6 +53,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 @@ -87,7 +88,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 @@ -131,6 +132,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 @@ -172,7 +239,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 @@ -201,7 +268,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 @@ -236,7 +304,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 @@ -271,7 +339,8 @@ public void testExpiredSession() throws Exception

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

assertNull(listener.ex);
assertNull(listener.attributeException);
assertNull(listener.accessTimeException);
}
finally
{
Expand All @@ -296,6 +365,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