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

Fixes and extra testing for EE9 ContextHandler class loading #9878

Merged
merged 20 commits into from
Jun 30, 2023
Merged

Fixes and extra testing for EE9 ContextHandler class loading #9878

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
f225fb0
Fix EE9 ContextHandler class loading when it is not the root handler.
lachlan-roberts Jun 6, 2023
24cb5db
Add test to reproduce classloader failures in 12.0.x.
lachlan-roberts Jun 6, 2023
3fd9ac2
Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12.0.x-…
lachlan-roberts Jun 6, 2023
c96fdd7
cleanup unused variables in nested ContextHandler
lachlan-roberts Jun 6, 2023
7eebfc1
fixes for ContextHandler lifecycle when called directly
lachlan-roberts Jun 7, 2023
f3b925a
PR #9878 - changes from review
lachlan-roberts Jun 9, 2023
a67e48d
Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12.0.x-…
lachlan-roberts Jun 14, 2023
4c1cf7d
PR #9878 - fixes for test failures
lachlan-roberts Jun 14, 2023
d00391e
Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12.0.x-…
lachlan-roberts Jun 15, 2023
e4c79c5
Scope the request to the current ContextHandlers ApiContext
lachlan-roberts Jun 15, 2023
5dcf022
use Request.setContext in all places done in Jetty-11
lachlan-roberts Jun 15, 2023
0dce49c
Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12.0.x-…
lachlan-roberts Jun 20, 2023
bf5e0e9
Maintain context path in original nested Requests pathInContext
lachlan-roberts Jun 20, 2023
8550493
decode Path in content for nested Request
lachlan-roberts Jun 21, 2023
57a8630
save some attributes for request log and error dispatch
lachlan-roberts Jun 21, 2023
44de5cd
fix NPE from Response.checkSameSite()
lachlan-roberts Jun 21, 2023
0ffeb87
use fields in nested Request instead of attributes for last context
lachlan-roberts Jun 22, 2023
d5c81f9
fix checkstyle error
lachlan-roberts Jun 22, 2023
6dd5fd7
Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12.0.x-…
lachlan-roberts Jun 26, 2023
8b0e28f
update javadoc for nested ContextHandler
lachlan-roberts Jun 27, 2023
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
176 changes: 150 additions & 26 deletions jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
import org.eclipse.jetty.server.Response;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.Session;
import org.eclipse.jetty.server.handler.ContextHandler.ScopedContext;
import org.eclipse.jetty.server.handler.ContextHandlerCollection;
import org.eclipse.jetty.server.handler.ContextRequest;
import org.eclipse.jetty.session.AbstractSessionManager;
Expand All @@ -89,6 +90,7 @@
import org.eclipse.jetty.util.annotation.ManagedObject;
import org.eclipse.jetty.util.component.DumpableCollection;
import org.eclipse.jetty.util.component.Environment;
import org.eclipse.jetty.util.component.LifeCycle;
import org.eclipse.jetty.util.resource.Resource;
import org.eclipse.jetty.util.resource.ResourceFactory;
import org.eclipse.jetty.util.resource.Resources;
Expand Down Expand Up @@ -117,6 +119,20 @@
* By default, the context is created with the {@link AllowedResourceAliasChecker} which is configured to allow symlinks. If
* this alias checker is not required, then {@link #clearAliasChecks()} or {@link #setAliasChecks(List)} should be called.
* </p>
* This handler can be invoked in 2 different ways:
* <ul>
* <li>
* If this is added directly as a {@link Handler} on the {@link Server} this will supply the {@link CoreContextHandler}
* associated with this {@link ContextHandler}. This will wrap the request to a {@link CoreContextRequest} and fall
* through to the {@code CoreToNestedHandler} which invokes the {@link HttpChannel} and this will eventually reach
* {@link ContextHandler#handle(String, Request, HttpServletRequest, HttpServletResponse)}.
* </li>
* <li>
* If this is nested inside another {@link ContextHandler} and not added directly to the server then its
* {@link CoreContextHandler} will never be added to the server. However it will still be created and its
* {@link ScopedContext} will be used to enter scope.
* </li>
* </ul>
*/
@ManagedObject("EE9 Context")
public class ContextHandler extends ScopedHandler implements Attributes, Supplier<Handler>
Expand Down Expand Up @@ -228,6 +244,12 @@ public ContextHandler(String contextPath)
this(null, null, contextPath);
}

public ContextHandler(String contextPath, org.eclipse.jetty.ee9.nested.Handler handler)
Copy link
Contributor

Choose a reason for hiding this comment

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

We need some javadoc somewhere that describes how this Context can be invoked in 2 different ways:

  1. via it's nested coreContextHandler which will wrap the core request/response and create the nest baseRequest/baseResponse
  2. via a direct call to handle when this handler is nested within another context handler.

In fact, I really don't like it that this class can simultaneously be used in both ways. In the case of a nested contextHandler, that means there is a second coreContextHandler just doing nothing. I'd prefer to have a mode where the second coreContextHandler was never created and only then could it receive requests from handle.

At the very least, this needs to be javadoc'd, but I'd prefer a mode.... I don't think splitting is possible.

Perhaps the mode can be established by a call to the get of the coreContextHandler?

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 think the split is possible. Maybe if the CoreContextHandler had a nested ContextHandler and delegated to that instead of the other way around.

Right now nested ContextHandler delegates lots of its methods to CoreContextHandler and its ScopedContext. So hard to separate them without some more fundamental changes.

Anyway this would be for another PR if we decide to do it. For now I have added some javadoc to explain the two different cases how this can be invoked.

{
this(contextPath);
setHandler(handler);
}

public ContextHandler(org.eclipse.jetty.server.Handler.Container parent)
{
this(null, parent, "/");
Expand Down Expand Up @@ -294,6 +316,8 @@ public void setAllowNullPathInfo(boolean allowNullPathInfo)
public void setServer(Server server)
{
super.setServer(server);
if (!Objects.equals(server, _coreContextHandler.getServer()))
_coreContextHandler.setServer(server);
if (_errorHandler != null)
_errorHandler.setServer(server);
}
Expand Down Expand Up @@ -581,13 +605,38 @@ public void setLogger(Logger logger)
@Override
protected void doStart() throws Exception
{
// If we are being started directly (rather than via a start of the CoreContextHandler), then
// we need to run ourselves in the core context
// If we are being started directly (rather than via a start of the CoreContextHandler),
// then we need the LifeCycle Listener to ensure both this and the CoreContextHandler are
// in STARTING state when doStartInContext is called.
if (org.eclipse.jetty.server.handler.ContextHandler.getCurrentContext() != _coreContextHandler.getContext())
{
// Make the CoreContextHandler lifecycle responsible for calling the doStartContext() and doStopContext().
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a thought... but would it be simpler to always have this handler as a non managed bean in the core context handler, and to always have a lifecycle listener registered that does the start and stop. That way this is the normal path rather than a transient strange path?

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that if we did it this way, we'd need to explicitly dump the context handler from the core context handler, so that we would get the deep dump, but I think that is simple to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lachlan-roberts thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry didn't see this comment.

I think even if we had the listener permanently registered we would still need this if statement to manually start the CoreContextHandler.

Something like this

if (org.eclipse.jetty.server.handler.ContextHandler.getCurrentContext() != _coreContextHandler.getContext())
     _coreContextHandler.start();
// else do nothing (don't call doStartInContext because listener will)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And then would the listener lifeCycleStarting be called before or after this doStart? I'm not sure.

We would need to ensure this is in STARTING state when the listener is called (not just the CoreContextHandler in STARTING state).

_coreContextHandler.unmanage(this);
_coreContextHandler.addEventListener(new LifeCycle.Listener()
{
@Override
public void lifeCycleStarting(LifeCycle event)
{
try
{
_coreContextHandler.getContext().call(() -> doStartInContext(), null);
}
catch (Exception e)
{
throw new RuntimeException(e);
}
}

@Override
public void lifeCycleStarted(LifeCycle event)
{
_coreContextHandler.manage(this);
_coreContextHandler.removeEventListener(this);
}
});

_coreContextHandler.start();
_coreContextHandler.manage(this);
return;
}

_coreContextHandler.getContext().call(this::doStartInContext, null);
Expand All @@ -613,14 +662,39 @@ protected void doStartInContext() throws Exception
@Override
protected void doStop() throws Exception
{
// If we are being stopped directly (rather than via a start of the CoreContextHandler), then
// we need to stop ourselves in the core context
// If we are being stopped directly (rather than via a start of the CoreContextHandler),
// then doStopInContext() will be called by the listener on the lifecycle of CoreContextHandler.
if (org.eclipse.jetty.server.handler.ContextHandler.getCurrentContext() != _coreContextHandler.getContext())
{
// Make the CoreContextHandler lifecycle responsible for calling the doStartContext() and doStopContext().
_coreContextHandler.unmanage(this);
_coreContextHandler.addEventListener(new LifeCycle.Listener()
{
@Override
public void lifeCycleStopping(LifeCycle event)
{
try
{
_coreContextHandler.getContext().call(() -> doStopInContext(), null);
}
catch (Exception e)
{
throw new RuntimeException(e);
}
}

@Override
public void lifeCycleStopped(LifeCycle event)
{
_coreContextHandler.manage(this);
_coreContextHandler.removeEventListener(this);
}
});

_coreContextHandler.stop();
_coreContextHandler.manage(this);
return;
}

_coreContextHandler.getContext().call(this::doStopInContext, null);
}

Expand Down Expand Up @@ -806,7 +880,65 @@ public void doScope(String target, Request baseRequest, HttpServletRequest reque
if (LOG.isDebugEnabled())
LOG.debug("scope {}|{}|{} @ {}", baseRequest.getContextPath(), baseRequest.getServletPath(), baseRequest.getPathInfo(), this);

nextScope(target, baseRequest, request, response);
APIContext oldContext = baseRequest.getContext();
String oldPathInContext = baseRequest.getPathInContext();
String pathInContext = target;
DispatcherType dispatch = baseRequest.getDispatcherType();

// Are we already in this context?
if (oldContext != _apiContext)
{
// check the target.
String contextPath = getContextPath();
if (DispatcherType.REQUEST.equals(dispatch) || DispatcherType.ASYNC.equals(dispatch))
{
if (target.length() > contextPath.length())
{
if (contextPath.length() > 1)
target = target.substring(contextPath.length());
pathInContext = target;
}
else if (contextPath.length() == 1)
{
target = "/";
pathInContext = "/";
}
else
{
target = "/";
pathInContext = null;
}
}
}

try
{
baseRequest.setContext(_apiContext,
(DispatcherType.INCLUDE.equals(dispatch) || !target.startsWith("/")) ? oldPathInContext : pathInContext);

ScopedContext context = getCoreContextHandler().getContext();
if (context == org.eclipse.jetty.server.handler.ContextHandler.getCurrentContext())
{
nextScope(target, baseRequest, request, response);
}
else
{
String t = target;
context.call(() -> nextScope(t, baseRequest, request, response), baseRequest.getCoreRequest());
}
}
catch (IOException | ServletException | RuntimeException e)
{
throw e;
}
catch (Throwable t)
{
throw new ServletException("Unexpected Exception", t);
}
finally
{
baseRequest.setContext(oldContext, oldPathInContext);
}
}

protected void requestInitialized(Request baseRequest, HttpServletRequest request)
Expand Down Expand Up @@ -926,8 +1058,6 @@ protected void exitScope(Request request)
*/
public void handle(Request request, Runnable runnable)
{
ClassLoader oldClassloader = null;
Thread currentThread = null;
APIContext oldContext = __context.get();

// Are we already in the scope?
Expand Down Expand Up @@ -1582,14 +1712,12 @@ public void handleAsync(HttpChannel channel) throws IOException, ServletExceptio
// this is a dispatch with either a provided URI and/or a dispatched path
// We will have to modify the request and then revert
final HttpURI oldUri = baseRequest.getHttpURI();
final String oldPathInContext = baseRequest.getPathInContext();
final ServletPathMapping oldServletPathMapping = baseRequest.getServletPathMapping();
final MultiMap<String> oldQueryParams = baseRequest.getQueryParameters();
try
{
if (encodedPathQuery == null)
{
baseRequest.onDispatch(baseUri, oldPathInContext);
baseRequest.setHttpURI(baseUri);
}
else
{
Expand All @@ -1614,23 +1742,18 @@ public void handleAsync(HttpChannel channel) throws IOException, ServletExceptio
builder.param(baseUri.getParam());
if (StringUtil.isEmpty(builder.getQuery()))
builder.query(baseUri.getQuery());
baseRequest.setHttpURI(builder);

HttpURI uri = builder.asImmutable();
String pathInContext = uri.getDecodedPath();
if (baseRequest.getContextPath().length() > 1)
pathInContext = pathInContext.substring(baseRequest.getContextPath().length());

baseRequest.onDispatch(uri, pathInContext);
if (baseUri.getQuery() != null && baseRequest.getQueryString() != null)
baseRequest.mergeQueryParameters(oldUri.getQuery(), baseRequest.getQueryString());
}

baseRequest.setContext(null, baseRequest.getHttpURI().getDecodedPath());
handleAsync(channel, event, baseRequest);
}
finally
{
baseRequest.onDispatch(oldUri, oldPathInContext);
baseRequest.setServletPathMapping(oldServletPathMapping);
baseRequest.setHttpURI(oldUri);
baseRequest.setQueryParameters(oldQueryParams);
baseRequest.resetParameters();
}
Expand Down Expand Up @@ -1658,7 +1781,7 @@ private void handleAsync(HttpChannel channel, AsyncContextEvent event, Request b
*/
public class APIContext implements ServletContext
{
private final org.eclipse.jetty.server.handler.ContextHandler.ScopedContext _coreContext;
private final ScopedContext _coreContext;
protected boolean _enabled = true; // whether or not the dynamic API is enabled for callers
protected boolean _extendedListenerTypes = false;
private int _effectiveMajorVersion = SERVLET_MAJOR_VERSION;
Expand Down Expand Up @@ -2309,7 +2432,7 @@ public static class CoreContextRequest extends ContextRequest
AbstractSessionManager.RequestedSession _requestedSession;

protected CoreContextRequest(org.eclipse.jetty.server.Request wrapped,
org.eclipse.jetty.server.handler.ContextHandler.ScopedContext context,
ScopedContext context,
HttpChannel httpChannel)
{
super(context, wrapped);
Expand Down Expand Up @@ -2426,7 +2549,8 @@ public void setContextPath(String contextPath)
protected void doStart() throws Exception
{
ClassLoader old = Thread.currentThread().getContextClassLoader();
Thread.currentThread().setContextClassLoader(ENVIRONMENT.getClassLoader());
if (getClassLoader() != null)
Thread.currentThread().setContextClassLoader(getClassLoader());
try
{
super.doStart();
Expand All @@ -2450,7 +2574,8 @@ public void createTempDirectory()
protected void doStop() throws Exception
{
ClassLoader old = Thread.currentThread().getContextClassLoader();
Thread.currentThread().setContextClassLoader(ENVIRONMENT.getClassLoader());
if (getClassLoader() != null)
Thread.currentThread().setContextClassLoader(getClassLoader());
try
{
super.doStop();
Expand Down Expand Up @@ -2486,7 +2611,7 @@ public void setServer(Server server)
}

@Override
protected CoreContext newContext()
protected ScopedContext newContext()
{
return new CoreContext();
}
Expand Down Expand Up @@ -2545,7 +2670,6 @@ protected void notifyExitScope(org.eclipse.jetty.server.Request coreRequest)

public ContextHandler getContextHandler()
{

return ContextHandler.this;
}

Expand Down
6 changes: 4 additions & 2 deletions jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Dispatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,8 @@ protected void forward(ServletRequest request, ServletResponse response, Dispatc
query = old_uri.getQuery();

String decodedPathInContext = URIUtil.decodePath(_pathInContext);
baseRequest.onDispatch(HttpURI.build(old_uri, _uri.getPath(), _uri.getParam(), query), decodedPathInContext);
baseRequest.setHttpURI(HttpURI.build(old_uri, _uri.getPath(), _uri.getParam(), query));
baseRequest.setContext(_contextHandler.getServletContext(), decodedPathInContext);

if (_uri.getQuery() != null || old_uri.getQuery() != null)
{
Expand Down Expand Up @@ -229,7 +230,8 @@ protected void forward(ServletRequest request, ServletResponse response, Dispatc
}
finally
{
baseRequest.onDispatch(old_uri, old_path_in_context);
baseRequest.setHttpURI(old_uri);
baseRequest.setContext(old_context, old_path_in_context);
baseRequest.setServletPathMapping(old_mapping);
baseRequest.setQueryParameters(old_query_params);
baseRequest.resetParameters();
Expand Down
3 changes: 2 additions & 1 deletion jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ErrorHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.util.stream.Collectors;

import jakarta.servlet.RequestDispatcher;
import jakarta.servlet.ServletContext;
import jakarta.servlet.ServletException;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
Expand Down Expand Up @@ -88,7 +89,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
// This logic really should be in ErrorPageErrorHandler, but some implementations extend ErrorHandler
// and implement ErrorPageMapper directly, so we do this here in the base class.
String errorPage = (this instanceof ErrorPageMapper) ? ((ErrorPageMapper)this).getErrorPage(request) : null;
ContextHandler.APIContext context = baseRequest.getErrorContext();
ServletContext context = baseRequest.getLastContext();
Dispatcher errorDispatcher = (errorPage != null && context != null)
? (Dispatcher)context.getRequestDispatcher(errorPage) : null;

Expand Down
2 changes: 1 addition & 1 deletion jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1014,7 +1014,7 @@ public void onCompleted()
{
CustomRequestLog.LogDetail logDetail = new CustomRequestLog.LogDetail(
_request.getServletName(),
_request.getServletContext().getRealPath(_request.getPathInContext())
_request.getLastContext().getRealPath(_request.getLastPathInContext())
);
_request.setAttribute(CustomRequestLog.LOG_DETAIL, logDetail);
}
Expand Down
3 changes: 2 additions & 1 deletion jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannelState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -922,7 +922,8 @@ public void sendError(int code, String message)
response.setStatus(code);
response.errorClose();

request.setAttribute(ErrorHandler.ERROR_CONTEXT, request.getErrorContext());
ServletContext errorContext = request.getLastContext();
request.setAttribute(ErrorHandler.ERROR_CONTEXT, errorContext);
request.setAttribute(ERROR_REQUEST_URI, request.getRequestURI());
request.setAttribute(ERROR_SERVLET_NAME, request.getServletName());
request.setAttribute(ERROR_STATUS_CODE, code);
Expand Down
Loading