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

Jump to bottom

implement servlet upgrade for ee10 #10128

Closed
wants to merge 22 commits into from
Closed

implement servlet upgrade for ee10 #10128

wants to merge 22 commits into from

Conversation

lachlan-roberts
Copy link
Contributor

implementing ee10 servlet upgrade for PR #10064

gregw and others added 9 commits July 5, 2023 15:52
@gregw
Various Cleanup in ServletChannel
66cc353 
Remove lambda for clarity
@gregw
Various Cleanup in ServletChannel
adb2fb2 
removed unused variable
@gregw
Various Cleanup in ServletChannel
a02dd13 
Requests are always handled by ServletHandler, one way or another.
@lachlan-roberts
make ServletChannel error dispatch async
8faf41e 
Signed-off-by: Lachlan Roberts <[email protected]>
@gregw
Various Cleanup in ServletChannel
de21dd7 
Servlet upgrade
@lachlan-roberts
scheduleDispatch after error handler callback
073e95a 
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts
use AtomicBoolean instead of AtomicInteger for error dispatch callback
c980ab5 
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts
remove checkAndPrepareUpgrade() from ServletChannel
0e2a0c1 
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts
implement servlet upgrade for ee10
68439bf 
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts
adjust implementation to fix tests
452a71b 
Signed-off-by: Lachlan Roberts <[email protected]>
@gregw gregw force-pushed the experiment/jetty-12-ServletChannel-cleanup branch from ee383a2 to 3d24929 Compare July 27, 2023 16:38
Base automatically changed from experiment/jetty-12-ServletChannel-cleanup to jetty-12.0.x August 15, 2023 10:02
@lachlan-roberts
Merge remote-tracking branch 'origin/jetty-12.0.x' into experiment/je…
b5ae22f 
…tty-12-ServletChannel-cleanup-ee10-servlet-upgrade
gregw
gregw requested changes Oct 3, 2023
View reviewed changes
.github/ISSUE_TEMPLATE/release-template.md Outdated Show resolved Hide resolved
@lachlan-roberts
Merge remote-tracking branch 'origin/jetty-12.0.x' into experiment/je…
7911d62 
…tty-12-ServletChannel-cleanup-ee10-servlet-upgrade
@lachlan-roberts
Merge remote-tracking branch 'origin/jetty-12.0.x' into experiment/je…
1869ca6 
…tty-12-ServletChannel-cleanup-ee10-servlet-upgrade
@joakime joakime requested a review from gregw February 21, 2024 22:02
@gregw
Copy link
Contributor

gregw commented May 23, 2024

@lachlan-roberts is this still a thing?

@lachlan-roberts
Copy link
Contributor Author

@gregw yes, but we need to discuss the issue with the callback.

  1. You say we should not execute the callback.
  2. Using SerializedInvoker like we did before causes it to block forever.
  3. Running the callback directly causes an infinite recursion.

So we need to think of some other solution for this.

@lachlan-roberts
Merge remote-tracking branch 'origin/jetty-12.0.x' into experiment/je…
b6286b2 
…tty-12-ServletChannel-cleanup-ee10-servlet-upgrade
gregw
gregw requested changes Jun 20, 2024
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

The executing of the write callback is still a hard blocker for me.

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java Outdated Show resolved Hide resolved
@lachlan-roberts
Copy link
Contributor Author

I am not executing this anymore, I have lots of changes which I have not pushed, will push them today.

@janbartel janbartel mentioned this pull request Jun 20, 2024
Closed
21 tasks
@janbartel
Copy link
Contributor

@lachlan-roberts we need this to pass the servlet 6.1 tck

@joakime joakime added Enhancement Specification For all industry Specifications (IETF / Servlet / etc) labels Jun 26, 2024
@janbartel
Copy link
Contributor

@lachlan-roberts nudge

@janbartel
Copy link
Contributor

@lachlan-roberts can you please resolve the conflicts and then ensure that we have the simplest implementation that we can get committed?

gregw
gregw requested changes Aug 20, 2024
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

Some of my comments are still pending... so I'm sending them again....

...-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java Show resolved Hide resolved
@lachlan-roberts lachlan-roberts mentioned this pull request Aug 21, 2024
Merged
@lachlan-roberts
Copy link
Contributor Author

replaced by #12186

@lachlan-roberts lachlan-roberts deleted the experiment/jetty-12-ServletChannel-cleanup-ee10-servlet-upgrade branch August 21, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joakime joakime joakime left review comments

@olamy olamy olamy left review comments

@gregw gregw gregw requested changes

@sbordet sbordet Awaiting requested review from sbordet

Assignees

@lachlan-roberts lachlan-roberts

Labels
Enhancement Specification For all industry Specifications (IETF / Servlet / etc)
Projects
Jetty 12.1.0
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lachlan-roberts @gregw @janbartel @olamy @joakime