Initial work for Cross Context Dispatch testing suite #11886
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...xt-dispatch/ccd-ee8-webapp/src/main/java/org/eclipse/jetty/tests/ccd/ee8/ForwardServlet.java
Dismissed
Show dismissed
Hide dismissed
janbartel
requested changes
Jun 8, 2024
| // ======================================================================== | ||
| // | ||
|
|
||
| package org.eclipse.jetty.tests.ccd.common; |
There was a problem hiding this comment.
Can this class be a record instead?
There was a problem hiding this comment.
our json lib didn't like that
There was a problem hiding this comment.
Must say something about the age of the json library :)
|
|
||
| package org.eclipse.jetty.tests.ccd.common; | ||
|
|
||
| public interface Step |
There was a problem hiding this comment.
The fashion these days appears to be to put any abstract classes and any impls in the same package inside of the interface.
There was a problem hiding this comment.
that seems noisy for no good reason.
There was a problem hiding this comment.
Take a look at the Handler class. I'm just getting in with this before Simone or Greg do :)
| import org.eclipse.jetty.tests.ccd.common.steps.GetHttpSessionStep; | ||
| import org.eclipse.jetty.tests.ccd.common.steps.RequestDispatchStep; | ||
|
|
||
| public class CCDServlet extends HttpServlet |
There was a problem hiding this comment.
It's possible to use the ee9 -> ee8 translation maven plugin to auto generate the classes, might want to consider that.
There was a problem hiding this comment.
No benefit for that.
First, this ccd-ee8-webapp is actually different than the ccd-ee9-webapp.
Also, there's no jetty-ee8 dependencies in this webapp for the translation plugin to work against.
It's a webapp, with jetty-core deps.
There was a problem hiding this comment.
This can easily be translated from the ccd-ee9-webapp to replace jakarta.servlet. with javax.servlet. Also I think both the ee8 and ee9 tests should do the same object wrapper identity test, so the code should be identical anyway.
| }, | ||
| "steps": [ | ||
| { | ||
| "class": "org.eclipse.jetty.tests.ccd.common.steps.ContextRedispatchStep", |
There was a problem hiding this comment.
As well as testing a straight Forward and Include (and then return) between ee8, ee9, ee10, I'd like us to also test the slightly more complex scenarios of :
Forward -> Forward -> return , Forward -> Include -> return , Include -> Forward -> return, Include -> Include -> return across ee8, ee9, ee10 and soon ee11.
There was a problem hiding this comment.
Sure. Can do.
There's many options we can wire up with this.
There was a problem hiding this comment.
We also need a test that does:
eeX creates a session, forwards to eeY, which creates a session, returns back to eeX, then eeX forwards again to eeY. Need to check that eeY finds the same session it previously created. We also need to check (somehow) that all sessions are correctly completed during the return from dispatch chain.
| "org.eclipse.jetty.tests.ccd.ee10.CCDServlet.service() dispatcherType=REQUEST method=GET requestUri=/ccd-ee10/redispatch/ee10", | ||
| "org.eclipse.jetty.tests.ccd.ee10.DumpServlet.service() dispatcherType=FORWARD method=GET requestUri=/ccd-ee10/dump/ee10" | ||
| ], | ||
| "expectedProperties": [ |
There was a problem hiding this comment.
I also want to test that if the jakarta.servlet.forward.* or jakarta.servlet.include.* attributes are requested in an ee9 and above context that originated from an dispatch from an ee8 context, that the correct values are returned (which would have been javax.servlet.forward.* or javax.servlet.include.* in the original ee8 context).
Similarly, if an ee9 context does a dispatch to an ee8 context, the ee8 context should be able to see the dispatch attributes with a prefix of javax.servlet instead of jakarta.servlet.
There was a problem hiding this comment.
Absolutely, those properties are present.
For example, the properties on this one are reported as ...
dispatchPlan.event[0]=Initial plan: context-ee10-forward-dump.json
dispatchPlan.event[1]=DispatchPlanHandler.handle() method=GET path-query=/ccd-ee10/redispatch/ee10
dispatchPlan.event[2]=org.eclipse.jetty.tests.ccd.ee10.CCDServlet.service() dispatcherType=REQUEST method=GET requestUri=/ccd-ee10/redispatch/ee10
dispatchPlan.event[3]=org.eclipse.jetty.tests.ccd.ee10.DumpServlet.service() dispatcherType=FORWARD method=GET requestUri=/ccd-ee10/dump/ee10
dispatchPlan.events.count=4
request.attr[jakarta.servlet.forward.context_path]=/ccd-ee10
request.attr[jakarta.servlet.forward.mapping]=ServletPathMapping{mappingMatch=PATH, matchValue=redispatch, pattern=/redispatch/*, servletName=ccd, servletPath=/redispatch, pathInfo=/ee10}
request.attr[jakarta.servlet.forward.path_info]=/ee10
request.attr[jakarta.servlet.forward.query_string]=<null>
request.attr[jakarta.servlet.forward.request_uri]=/ccd-ee10/redispatch/ee10
request.attr[jakarta.servlet.forward.servlet_path]=/redispatch
request.attr[org.eclipse.jetty.server.Request.Cookies]=[]
request.attr[org.eclipse.jetty.server.Request.maxFormContentSize]=200000
request.attr[org.eclipse.jetty.server.Request.maxFormKeys]=1000
request.attr[org.eclipse.jetty.tests.ccd.common.DispatchPlan]=DispatchPlan[id=context-ee10-forward-dump.json]
request.authType=<null>
request.characterEncoding=<null>
request.contentLength=-1
request.contentType=<null>
request.contextPath=/ccd-ee10
request.dispatcherType=FORWARD
request.header[Accept-Encoding]=gzip
request.header[Host]=localhost:33753
request.header[User-Agent]=Jetty/12.0.11-SNAPSHOT
request.header[X-DispatchPlan]=context-ee10-forward-dump.json
request.localAddr=127.0.0.1
request.localName=127.0.0.1
request.localPort=33753
request.locale=en_US
request.method=GET
request.pathInfo=/ee10
request.pathTranslated=<null>
request.protocol=HTTP/1.1
request.queryString=<null>
request.remoteAddr=127.0.0.1
request.remoteHost=127.0.0.1
request.remotePort=45628
request.remoteUser=<null>
request.requestURI=/ccd-ee10/dump/ee10
request.requestURL=http://localhost:33753/ccd-ee10/dump/ee10
request.requestedSessionId=<null>
request.serverPort=33753
request.servletPath=/dump
| ], | ||
| "expectedProperties": [ | ||
| { | ||
| "class": "org.eclipse.jetty.tests.ccd.common.Property", |
There was a problem hiding this comment.
We also need to be able to test object wrapper identity (see #11888). So we need a test that wraps the request before the dispatch, and inside the dispatch target we see exactly the same object.
There was a problem hiding this comment.
That's done in a different testcase right now.
See RedispatchTests.testEe8FilterWithAwkwardRequestURI()
There was a problem hiding this comment.
I don't think that test is actually testing object wrapper identity with an == but rather inferring it: I think Greg has said he'd like to see an actual instance equality test.
| * {@link HttpServletRequest#getRequestURI()} to something that does | ||
| * not satisfy the Servlet spec URI invariant {@code request URI == context path + servlet path + path info} | ||
| */ | ||
| public class InternalRequestURIFilter implements Filter |
There was a problem hiding this comment.
Is this attempting to test the object wrapper identity that I mentioned in a subsequent comment? If so I think we need a different test that checks exact object equality.
janbartel
requested changes
Jun 11, 2024
| import org.eclipse.jetty.tests.ccd.common.steps.GetHttpSessionStep; | ||
| import org.eclipse.jetty.tests.ccd.common.steps.RequestDispatchStep; | ||
|
|
||
| public class CCDServlet extends HttpServlet |
There was a problem hiding this comment.
This can easily be translated from the ccd-ee9-webapp to replace jakarta.servlet. with javax.servlet. Also I think both the ee8 and ee9 tests should do the same object wrapper identity test, so the code should be identical anyway.
|
@janbartel here's that stacktrace you were asking about. |
|
@joakime I've merged your branch into the https://github.com/jetty/jetty.project/tree/jetty-12.0.x-object-identity-ee8-and-ee9 so I can do some testing. It helped me find the bug that was causing the stacktrace you posted earlier! |
janbartel
requested changes
Jun 13, 2024
| } | ||
| else if (step instanceof Step.HttpSessionSetAttribute sessionSetAttribute) | ||
| { | ||
| HttpSession session = req.getSession(true); |
There was a problem hiding this comment.
I'm putting this comment on the ee10 CCDServlet, but it goes for all the others as well. I think each step needs to generate a dispatchPlan.addEvent. I was just following through the event output from one of the tests and I got confused as it looked like there was a "HttpSession exists: null" where it shouldn't be, but then I realized that the HttpSetSessionAttribute step doesn't generate an event output. I think its important for consistency and comprehensibility that every step generates at least 1 event output.
For extra credit, it might be nice to put the generation of those events inside the Step itself, so each servlet doesn't haven't to do so much boilerplate code (with the possibility of making errors), so something like:
interface Step
{
void addEvent(DispatchPlan plan, String... args);
}
class GetHttpSession extends AbstractStep
{
public void addEvent(DispatchPlan plan, String... args)
{
plan.addEvent("%s.service() HttpSession exists: [%s]=[%s]",
args[0],
args[1],
args[2]
}
}so then the CCDServlets code becomes:
step.addEvent(
dispatchPlan,
this.getClass().getName(),
name,
Objects.toString(value));| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| public class PlanSessionCache extends DefaultSessionCache |
There was a problem hiding this comment.
Add a comment about why this is necessary
| import java.util.List; | ||
| import java.util.concurrent.LinkedBlockingDeque; | ||
|
|
||
| public class DispatchPlan |
|
|
||
| import java.util.Map; | ||
|
|
||
| public class HttpRequest implements Step |
janbartel
requested changes
Jun 14, 2024
| // we should have the commit() and release() for each new Session. | ||
| for (String sessionId : newSessions) | ||
| { | ||
| assertThat(logEntries, hasItem("SessionCache.event.commit()=" + sessionId)); |
There was a problem hiding this comment.
I think there's a race condition here - I think that the log might not be finished being written by the time you're looking at the output, as I see tests of these session contents failing randomly, even though the log contains the correct information. So you need to use Awaitility to wait a bit. Have a look for example at https://github.com/jetty/jetty.project/blob/jetty-12.0.x/jetty-ee9/jetty-ee9-tests/jetty-ee9-test-sessions/jetty-ee9-test-sessions-common/src/test/java/org/eclipse/jetty/ee9/session/CreationTest.java#L157
There was a problem hiding this comment.
In this test suite we cannot dig down into a class in one of the EE# environments (we don't have access to it).
We don't even have access to that JVM for waiting on the Server instance, as it's a forked JVM.
That means we have no test we can give Awaitility.
The best we can do is setup graceful shutdown and wait on the JVM closing via the JettyHomeTester.
There was a problem hiding this comment.
All you need to do is to wait until the session.log file is no longer being written to.
There was a problem hiding this comment.
All you need to do is to wait until the
session.logfile is no longer being written to.
That's not a state check you can do from a different process (pid).
Keep in mind we are dealing with a forked JVM.
Waiting for the forked JVM to exit seems like the only event we can use.
janbartel
previously approved these changes
Jun 19, 2024
joakime
force-pushed
the
fix/12.0.x/cross-context-testing-2
branch
from
044b9d9
to
7011827
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cross Context Dispatch testing suite across multiple Jetty EE Environments.
This replaces PR #11885