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.

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

Undertow - Session context lifecycle events should be observable from session scoped beans #46364

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ public class SessionContextTestCase {

@RegisterExtension
static QuarkusUnitTest runner = new QuarkusUnitTest()
.withApplicationRoot((jar) -> jar.addClasses(TestServlet.class, Foo.class, ObservingBean.class));
.withApplicationRoot(
(jar) -> jar.addClasses(TestServlet.class, Foo.class, ObservingBean.class, SessionScopedObserver.class));

@Test
public void testServlet() {
Expand All @@ -38,12 +39,17 @@ public void testContextEvents() {

// make sure we start with zero events to keep this test method independent
observingBean.resetState();
SessionScopedObserver.resetState();

// following request creates a session and also destroys it by enforcing invalidation
when().get("/foo?destroy=true").then().statusCode(200);
Assertions.assertEquals(1, observingBean.getTimesInitObserved());
Assertions.assertEquals(1, observingBean.getTimesBeforeDestroyedObserved());
Assertions.assertEquals(1, observingBean.getTimesDestroyedObserved());

// assert that @SessionScoped bean can observe init and before destroyed events as well
Assertions.assertEquals(1, SessionScopedObserver.timesInitObserved);
Assertions.assertEquals(1, SessionScopedObserver.timesBeforeDestroyedObserved);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package io.quarkus.undertow.test.sessioncontext;

import jakarta.enterprise.context.BeforeDestroyed;
import jakarta.enterprise.context.Initialized;
import jakarta.enterprise.context.SessionScoped;
import jakarta.enterprise.event.Observes;

@SessionScoped
public class SessionScopedObserver {

static int timesInitObserved = 0;
static int timesBeforeDestroyedObserved = 0;

public void observeInit(@Observes @Initialized(SessionScoped.class) Object event) {
timesInitObserved++;
}

public void observeBeforeDestroyed(@Observes @BeforeDestroyed(SessionScoped.class) Object event) {
timesBeforeDestroyedObserved++;
}

public static void resetState() {
timesInitObserved = 0;
timesBeforeDestroyedObserved = 0;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public class HttpSessionContext implements InjectableContext, HttpSessionListene
+ ".contextualInstances";

private static final ThreadLocal<HttpSession> DESTRUCT_SESSION = new ThreadLocal<>();
private static final ThreadLocal<HttpSession> CREATED_SESSION = new ThreadLocal<>();

private static final Logger LOG = Logger.getLogger(HttpSessionContext.class);

Expand Down Expand Up @@ -160,7 +161,10 @@ private ComputingCache<Key, ContextInstanceHandle<?>> getContextualInstances(Htt
}

private HttpSession session(boolean create) {
HttpSession session = null;
HttpSession session = CREATED_SESSION.get();
if (session != null) {
return session;
}
try {
session = ((HttpServletRequest) ServletRequestContext.requireCurrent().getServletRequest()).getSession(create);
} catch (IllegalStateException ignored) {
Expand Down Expand Up @@ -213,6 +217,7 @@ public void sessionDestroyed(HttpSessionEvent se) {
event.select(HttpSession.class, BeforeDestroyed.Literal.SESSION).fire(session);
try {
DESTRUCT_SESSION.set(session);
CREATED_SESSION.remove();
destroy(session);
event.select(HttpSession.class, Destroyed.Literal.SESSION).fire(session);
} finally {
Expand All @@ -222,7 +227,9 @@ public void sessionDestroyed(HttpSessionEvent se) {

@Override
public void sessionCreated(HttpSessionEvent se) {
Arc.container().beanManager().getEvent().select(HttpSession.class, Initialized.Literal.SESSION).fire(se.getSession());
HttpSession createdSession = se.getSession();
CREATED_SESSION.set(createdSession);
Arc.container().beanManager().getEvent().select(HttpSession.class, Initialized.Literal.SESSION).fire(createdSession);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we need a thread local because at this point the ServletRequestContext.requireCurrent().getServletRequest()).getSession(true) returns null or a different session?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A different session - with true it always creates a new one.
Plus if you try the same invocation with false (to see the session for which you just called session created callback), you will get null at this point 🤷
So we need some way to remember that we already created a session - at least that was my thinking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A different session - with true it always creates a new one.

That's weird, it should only create a new one if it does not exist, right? And given that it's called from jakarta.servlet.http.HttpSessionListener.sessionCreated(HttpSessionEvent) I would expect the session to exist...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's weird, it should only create a new one if it does not exist, right?

That is pretty much my yesterday thinking in a nutshell :)
Somehow, it ends up creating new session anyway.
You can debug this all the way to io.undertow.servlet.spec.ServletContextImpl#getSession where undertow tries to retrieve existing session first but I didn't manage to understand why it doesn't find it or whether it is right or wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can crack this with our knowledge of servlet/undertow...
I've sent an email question to @fl4via who should hopefully know more about Undertow and how it should behave it this case.

}

}