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

Test for ee10 response commit #9048

Merged
merged 7 commits into from
Dec 22, 2022
Merged

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Dec 13, 2022

This is a test and a partial fix demonstration that the response committed state is not correctly handled in ee10.

Currently the ServletRequestState class tracks the OutputState which includes the COMMITTED state. But currently nothing calls org.eclipse.jetty.ee10.servlet.ServletRequestState#commitResponse, so that state is never entered.
The partial fix is to use the committed state from the core request. If this is sufficient then the OutputState needs to be reviewed to see what states it should track.

@gregw
Copy link
Contributor Author

gregw commented Dec 13, 2022

@lachlan-roberts @janbartel do either one of you want to take this one?
@olamy FYI

@gregw
Copy link
Contributor Author

gregw commented Dec 13, 2022

@lachlan-roberts @janbartel I think ServletRequestState needs a good review as there are many warnings and a few methods that are never called.

@lachlan-roberts
Copy link
Contributor

@gregw were you expecting the test cases to fail here?
They pass for me.

@lachlan-roberts lachlan-roberts self-assigned this Dec 13, 2022
@gregw
Copy link
Contributor Author

gregw commented Dec 13, 2022

@lachlan-roberts my change to use the core isCommitted state fixed the test.... but it leaves kruft in HttpOutput, as the committed state is still there and not used... as is a method for intermediate 1xx responses reopening the printwriter after commit. So it needs review and cleanup

@gregw
Copy link
Contributor Author

gregw commented Dec 13, 2022

@lachlan-roberts can you also review org.eclipse.jetty.ee10.servlet.ServletRequestState#asyncError which is not called. Need to check if that is correct and that onError is correctly called when necessary.

@joakime joakime added this to the 12.0.x milestone Dec 14, 2022
// Release the chunk immediately, if it is empty.
if (!_chunk.hasRemaining() && !_chunk.isTerminal())
if (_chunk != null && !_chunk.hasRemaining() && !_chunk.isTerminal())
Copy link
Contributor

Choose a reason for hiding this comment

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

@sbordet can you review this change.
Looks like if we ever hit line 281 then _chunk will be null here.

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 we need to avoid using isTerminal as a condition for releasing. We should just release any chunk we are given.

@lachlan-roberts
Copy link
Contributor

@gregw can you review, can't request it through github because you created the PR.
I opened separate issue for #9060.

@gregw
Copy link
Contributor Author

gregw commented Dec 16, 2022

@lachlan-roberts I can't review as I created the PR. Other than the question about releasing terminal chunks (which @sbordet is addressing in another PR anyway), I think this is good. So if you think so too, then you can approve and I will merge.

@olamy
Copy link
Member

olamy commented Dec 16, 2022

@lachlan-roberts @gregw I have just tested this PR with TCK and I get this error

Exception in thread "TestTimer" java.lang.NullPointerException: Cannot invoke "org.eclipse.jetty.server.handler.ContextHandler$ScopedContext.run(java.lang.Runnable)" because the return value of "org.eclipse.jetty.ee10.servlet.AsyncContextEvent.getContext()" is null
	at org.eclipse.jetty.ee10.servlet.ServletRequestState.runInContext(ServletRequestState.java:1148)
	at org.eclipse.jetty.ee10.servlet.ServletRequestState.complete(ServletRequestState.java:692)
	at org.eclipse.jetty.ee10.servlet.AsyncContextState.complete(AsyncContextState.java:61)
	at com.sun.ts.tests.servlet.spec.async.TestServlet$1.run(TestServlet.java:47)
	at java.base/java.util.TimerThread.mainLoop(Timer.java:566)
	at java.base/java.util.TimerThread.run(Timer.java:516)

@lachlan-roberts
Copy link
Contributor

@olamy can you please test this again with TCK

@olamy
Copy link
Member

olamy commented Dec 21, 2022

@olamy can you please test this again with TCK

no more NPE but this increase the failure number by 7

@gregw
Copy link
Contributor Author

gregw commented Dec 21, 2022

@olamy can you link us to the new failures?

@gregw
Copy link
Contributor Author

gregw commented Dec 21, 2022

@olamy I just synced this branch with head, so that might fix some unrelated failures?

@olamy
Copy link
Member

olamy commented Dec 21, 2022

@gregw re run TCK with synced branch and definitely get better result!

Screenshot 2022-12-22 at 7 52 15 am

@gregw gregw requested a review from janbartel December 22, 2022 01:59
@gregw gregw merged commit 040ed85 into jetty-12.0.x Dec 22, 2022
@gregw gregw deleted the jetty-12-ee10-servlet-commit branch December 22, 2022 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants