Skip to content

Commit 087f486

Browse files
janbarteljanbartel
authored
Issue #6277 Better handling of exceptions thrown in sessionDestroyed (#6278) (#6279)
* Issue #6277 Better handling of exceptions thrown in sessionDestroyed Signed-off-by: Jan Bartel <janb@webtide.com>
1 parent edcaf70 commit 087f486

File tree

4 files changed

+119
-19
lines changed

4 files changed

+119
-19
lines changed

‎jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -498,10 +498,7 @@ public long getLastAccessedTime()
498498
{
499499
try (Lock lock = _lock.lock())
500500
{
501-
if (isInvalid())
502-
{
503-
throw new IllegalStateException("Session not valid");
504-
}
501+
checkValidForRead();
505502
return _sessionData.getLastAccessed();
506503
}
507504
}
@@ -947,14 +944,18 @@ public void invalidate()
947944
// do the invalidation
948945
_handler.callSessionDestroyedListeners(this);
949946
}
947+
catch (Exception e)
948+
{
949+
LOG.warn("Error during Session destroy listener", e);
950+
}
950951
finally
951952
{
952953
// call the attribute removed listeners and finally mark it
953954
// as invalid
954955
finishInvalidate();
956+
// tell id mgr to remove sessions with same id from all contexts
957+
_handler.getSessionIdManager().invalidateAll(_sessionData.getId());
955958
}
956-
// tell id mgr to remove sessions with same id from all contexts
957-
_handler.getSessionIdManager().invalidateAll(_sessionData.getId());
958959
}
959960
}
960961
catch (Exception e)

‎tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/TestHttpSessionListener.java

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,16 +31,18 @@ public class TestHttpSessionListener implements HttpSessionListener
3131
public List<String> createdSessions = new ArrayList<>();
3232
public List<String> destroyedSessions = new ArrayList<>();
3333
public boolean accessAttribute = false;
34-
public Exception ex = null;
34+
public boolean lastAccessTime = false;
35+
public Exception attributeException = null;
36+
public Exception accessTimeException = null;
3537

36-
public TestHttpSessionListener(boolean access)
38+
public TestHttpSessionListener(boolean accessAttribute, boolean lastAccessTime)
3739
{
38-
accessAttribute = access;
40+
this.accessAttribute = accessAttribute;
41+
this.lastAccessTime = lastAccessTime;
3942
}
4043

4144
public TestHttpSessionListener()
4245
{
43-
accessAttribute = false;
4446
}
4547

4648
public void sessionDestroyed(HttpSessionEvent se)
@@ -54,7 +56,19 @@ public void sessionDestroyed(HttpSessionEvent se)
5456
}
5557
catch (Exception e)
5658
{
57-
ex = e;
59+
attributeException = e;
60+
}
61+
}
62+
63+
if (lastAccessTime)
64+
{
65+
try
66+
{
67+
se.getSession().getLastAccessedTime();
68+
}
69+
catch (Exception e)
70+
{
71+
accessTimeException = e;
5872
}
5973
}
6074
}

‎tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/TestHttpSessionListenerWithWebappClasses.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,9 @@ public TestHttpSessionListenerWithWebappClasses()
3535
super();
3636
}
3737

38-
public TestHttpSessionListenerWithWebappClasses(boolean access)
38+
public TestHttpSessionListenerWithWebappClasses(boolean attribute, boolean lastAccessTime)
3939
{
40-
super(access);
40+
super(attribute, lastAccessTime);
4141
}
4242

4343
@Override
@@ -52,7 +52,7 @@ public void sessionDestroyed(HttpSessionEvent se)
5252
}
5353
catch (Exception cnfe)
5454
{
55-
ex = cnfe;
55+
attributeException = cnfe;
5656
}
5757
super.sessionDestroyed(se);
5858
}

‎tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/SessionListenerTest.java

Lines changed: 90 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
import static org.hamcrest.Matchers.in;
5959
import static org.hamcrest.Matchers.is;
6060
import static org.junit.jupiter.api.Assertions.assertEquals;
61+
import static org.junit.jupiter.api.Assertions.assertFalse;
6162
import static org.junit.jupiter.api.Assertions.assertNotEquals;
6263
import static org.junit.jupiter.api.Assertions.assertNotNull;
6364
import static org.junit.jupiter.api.Assertions.assertNull;
@@ -92,7 +93,7 @@ public void testListenerWithInvalidation() throws Exception
9293
TestServer server = new TestServer(0, inactivePeriod, scavengePeriod,
9394
cacheFactory, storeFactory);
9495
ServletContextHandler context = server.addContext(contextPath);
95-
TestHttpSessionListener listener = new TestHttpSessionListener(true);
96+
TestHttpSessionListener listener = new TestHttpSessionListener(true, true);
9697
context.getSessionHandler().addEventListener(listener);
9798
TestServlet servlet = new TestServlet();
9899
ServletHolder holder = new ServletHolder(servlet);
@@ -136,6 +137,72 @@ public void testListenerWithInvalidation() throws Exception
136137
LifeCycle.stop(server);
137138
}
138139
}
140+
141+
/**
142+
* Test that if a session listener throws an exception during sessionDestroyed the session is still invalidated
143+
*/
144+
@Test
145+
public void testListenerWithInvalidationException() throws Exception
146+
{
147+
String contextPath = "";
148+
String servletMapping = "/server";
149+
int inactivePeriod = 6;
150+
int scavengePeriod = -1;
151+
152+
DefaultSessionCacheFactory cacheFactory = new DefaultSessionCacheFactory();
153+
cacheFactory.setEvictionPolicy(SessionCache.NEVER_EVICT);
154+
TestSessionDataStoreFactory storeFactory = new TestSessionDataStoreFactory();
155+
storeFactory.setGracePeriodSec(scavengePeriod);
156+
157+
TestServer server = new TestServer(0, inactivePeriod, scavengePeriod,
158+
cacheFactory, storeFactory);
159+
ServletContextHandler context = server.addContext(contextPath);
160+
ThrowingSessionListener listener = new ThrowingSessionListener();
161+
context.getSessionHandler().addEventListener(listener);
162+
TestServlet servlet = new TestServlet();
163+
ServletHolder holder = new ServletHolder(servlet);
164+
context.addServlet(holder, servletMapping);
165+
166+
try
167+
{
168+
server.start();
169+
int port1 = server.getPort();
170+
171+
HttpClient client = new HttpClient();
172+
client.start();
173+
try
174+
{
175+
String url = "http://localhost:" + port1 + contextPath + servletMapping;
176+
// Create the session
177+
ContentResponse response1 = client.GET(url + "?action=init");
178+
assertEquals(HttpServletResponse.SC_OK, response1.getStatus());
179+
String sessionCookie = response1.getHeaders().get("Set-Cookie");
180+
assertNotNull(sessionCookie);
181+
assertTrue(TestServlet.bindingListener.bound);
182+
183+
String sessionId = TestServer.extractSessionId(sessionCookie);
184+
185+
// Make a request which will invalidate the existing session
186+
Request request2 = client.newRequest(url + "?action=test");
187+
ContentResponse response2 = request2.send();
188+
assertEquals(HttpServletResponse.SC_OK, response2.getStatus());
189+
190+
assertTrue(TestServlet.bindingListener.unbound);
191+
192+
//check session no longer exists
193+
assertFalse(context.getSessionHandler().getSessionCache().contains(sessionId));
194+
assertFalse(context.getSessionHandler().getSessionCache().getSessionDataStore().exists(sessionId));
195+
}
196+
finally
197+
{
198+
LifeCycle.stop(client);
199+
}
200+
}
201+
finally
202+
{
203+
LifeCycle.stop(server);
204+
}
205+
}
139206

140207
/**
141208
* Test that listeners are called when a session expires
@@ -177,7 +244,7 @@ public void testSessionExpiresWithListener() throws Exception
177244
ServletContextHandler context = server1.addContext(contextPath);
178245
context.setClassLoader(contextClassLoader);
179246
context.addServlet(holder, servletMapping);
180-
TestHttpSessionListener listener = new TestHttpSessionListenerWithWebappClasses(true);
247+
TestHttpSessionListener listener = new TestHttpSessionListenerWithWebappClasses(true, true);
181248
context.getSessionHandler().addEventListener(listener);
182249

183250
try
@@ -206,7 +273,8 @@ public void testSessionExpiresWithListener() throws Exception
206273

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

209-
assertNull(listener.ex);
276+
assertNull(listener.attributeException);
277+
assertNull(listener.accessTimeException);
210278
}
211279
finally
212280
{
@@ -241,7 +309,7 @@ public void testExpiredSession() throws Exception
241309
ServletHolder holder = new ServletHolder(servlet);
242310
ServletContextHandler context = server1.addContext(contextPath);
243311
context.addServlet(holder, servletMapping);
244-
TestHttpSessionListener listener = new TestHttpSessionListener();
312+
TestHttpSessionListener listener = new TestHttpSessionListener(true, true);
245313

246314
context.getSessionHandler().addEventListener(listener);
247315

@@ -276,7 +344,8 @@ public void testExpiredSession() throws Exception
276344

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

279-
assertNull(listener.ex);
347+
assertNull(listener.attributeException);
348+
assertNull(listener.accessTimeException);
280349
}
281350
finally
282351
{
@@ -301,6 +370,22 @@ public void sessionDestroyed(HttpSessionEvent se)
301370
{
302371
}
303372
}
373+
374+
public static class ThrowingSessionListener implements HttpSessionListener
375+
{
376+
377+
@Override
378+
public void sessionCreated(HttpSessionEvent se)
379+
{
380+
}
381+
382+
@Override
383+
public void sessionDestroyed(HttpSessionEvent se)
384+
{
385+
throw new IllegalStateException("Exception during sessionDestroyed");
386+
}
387+
388+
}
304389

305390
@Test
306391
public void testSessionListeners()

0 commit comments

Comments
 (0)