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

Copy ServletHolder class/instance properly during startWebapp #6214

Merged
merged 10 commits into from May 16, 2021

Conversation

@joakime
Copy link
Member

@joakime joakime commented Apr 26, 2021

#6280

joakime added 5 commits Apr 21, 2021
…tor.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
- using status code 555 for testcase
  eliminating 503 false success
  from different code path than
  what we are attempting to fix

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
- ServletHolder needs a mapping
  to allow RejectUncompiledJspServlet
  to respond to requests for JSPs
  that haven't been precompiled.
- Precompiled test class is now
  properly placed into a generated
  webapp base directory without
  contaminating the webapp classloader
  with other src/test/java classes

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime requested a review from gregw Apr 26, 2021
@joakime joakime self-assigned this Apr 26, 2021
gregw
gregw previously requested changes Apr 27, 2021
Copy link
Contributor

@gregw gregw left a comment

@janbartel can you review this and then let's chat. I think we need to think very carefully about what it means to have 2 holders sharing the same instance.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw requested a review from janbartel Apr 27, 2021
@gregw gregw marked this pull request as ready for review Apr 27, 2021
@gregw gregw dismissed their stale review Apr 27, 2021

I'm now an author

Signed-off-by: Greg Wilkins <gregw@webtide.com>
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw requested a review from janbartel Apr 28, 2021
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Copy link
Contributor

@janbartel janbartel left a comment

Both of the remaining test servlet classes could be inner classes too.

@@ -0,0 +1,37 @@
//
Copy link
Contributor

@janbartel janbartel Apr 29, 2021

Choose a reason for hiding this comment

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

Make this an inner class.

@@ -0,0 +1,37 @@
//
Copy link
Contributor

@janbartel janbartel Apr 29, 2021

Choose a reason for hiding this comment

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

Make this one an inner class of ForcedServletTest too.

@gregw gregw added this to In progress in Jetty 9.4.41 via automation May 15, 2021
use inner classes

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw requested a review from janbartel May 15, 2021
Copy link
Contributor

@janbartel janbartel left a comment

LGTM

Jetty 9.4.41 automation moved this from In progress to Reviewer approved May 15, 2021
@gregw gregw merged commit edcaf70 into jetty-9.4.x May 16, 2021
2 checks passed
Jetty 9.4.41 automation moved this from Reviewer approved to Done May 16, 2021
@gregw gregw deleted the jetty-9.4.x-jsp-precompile-alt-servlet branch May 16, 2021
@gregw gregw linked an issue that may be closed by this pull request May 16, 2021
gregw added a commit that referenced this issue May 16, 2021
 ServletHolder.copyClassServlet() method added to correctly copy held class.


Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw added a commit that referenced this issue May 16, 2021
Copy ServletHolder class/instance properly during startWebapp (#6214)
ServletHolder.copyClassServlet() method added to correctly copy held class.
Cherry picked from 9.4

Signed-off-by: Greg Wilkins <gregw@webtide.com>
Co-authored-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Linked issues

Successfully merging this pull request may close these issues.

3 participants