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

Various Cleanup in ServletChannel #10064

Merged
merged 13 commits into from
Aug 15, 2023

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Jul 5, 2023

Various Cleanup in ServletChannel

  • remove lambdas for clarity
  • TODO Non-blocking error dispatch
  • TODO isHandled does not exist
  • TODO checkAndPrepareUpgrade and implement servlet upgrade?
  • remove unused variables
  • review javadoc (including any warnings)
  • review any compiler or findbug warnings (if any)

@gregw
Copy link
Contributor Author

gregw commented Jul 5, 2023

@lachlan-roberts can you collaborate with me on this one.

@gregw gregw closed this Jul 27, 2023
@gregw gregw force-pushed the experiment/jetty-12-ServletChannel-cleanup branch from ee383a2 to 3d24929 Compare July 27, 2023 16:38
@gregw
Copy link
Contributor Author

gregw commented Jul 27, 2023

This got too out of date. starting again....

@gregw
Copy link
Contributor Author

gregw commented Jul 27, 2023

attempt two

@gregw gregw reopened this Jul 27, 2023
We will always handle because we would not get here if there was not a matching servlet and a noop matching servlet is just a 200 response.
Removed unused upgrade method
Improved javadoc.
Cleanup recycle.
Ensure recycle
Comment on lines 122 to 124
// TODO This is needed as requests that are handled before the ServletHandler are not recycled.
if (_servletContextRequest != null)
recycle();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We really need a cheap and easy mechanism to trigger on request completion... i.e both callback complete and handle method exited.

Fix recycle order
@gregw gregw marked this pull request as ready for review August 2, 2023 15:15
@gregw
Copy link
Contributor Author

gregw commented Aug 9, 2023

@lachlan-roberts nudge

@gregw gregw requested a review from janbartel August 11, 2023 01:21
@gregw gregw dismissed stale reviews from janbartel and lachlan-roberts via ac841ac August 11, 2023 07:52
@gregw gregw merged commit cdf5035 into jetty-12.0.x Aug 15, 2023
2 checks passed
@gregw gregw deleted the experiment/jetty-12-ServletChannel-cleanup branch August 15, 2023 10:02
@joakime joakime added the Specification For all industry Specifications (IETF / Servlet / etc) label Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Specification For all industry Specifications (IETF / Servlet / etc)
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants