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

Copy ServletHolder class/instance properly during startWebapp #6214

Merged
merged 10 commits into from
May 16, 2021
Merged

Copy ServletHolder class/instance properly during startWebapp #6214

merged 10 commits into from
May 16, 2021

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Apr 26, 2021 •
edited by gregw
Loading

@joakime
Attempt to replicate bug reported against precompiled jsps in descrip…
ca82dde 
…tor.

Signed-off-by: Joakim Erdfelt <[email protected]>
@joakime
Speculative ServletHolder.copyClassServlet() method added.
0032484 
Signed-off-by: Joakim Erdfelt <[email protected]>
@joakime
Adding System/Server ClasspathPattern fix
4424bfc 
Signed-off-by: Joakim Erdfelt <[email protected]>
@joakime
Updating testcase to eliminate false success.
0d95ef6 
- 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 <[email protected]>
@joakime
Updating testcase to fix issue.
4dbb146 
- 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 <[email protected]>
@joakime joakime requested a review from gregw April 26, 2021 22:49
@joakime joakime self-assigned this Apr 26, 2021
gregw
gregw previously requested changes Apr 27, 2021
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.

@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.

jetty-webapp/src/test/java/org/eclipse/jetty/webapp/ForcedServletTest.java Outdated Show resolved Hide resolved
jetty-webapp/src/test/java/org/eclipse/jetty/webapp/ForcedServletTest.java Outdated Show resolved Hide resolved
jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java Outdated Show resolved Hide resolved
@gregw
Copy just the class
6df2cf2 
Signed-off-by: Greg Wilkins <[email protected]>
@gregw gregw requested a review from janbartel April 27, 2021 04:39
@gregw gregw marked this pull request as ready for review April 27, 2021 04:40
@gregw gregw dismissed their stale review April 27, 2021 04:40

I'm now an author

@gregw
Simplified test
ce3df58 
Signed-off-by: Greg Wilkins <[email protected]>
@gregw
Simplified test
c29cb55 
Signed-off-by: Greg Wilkins <[email protected]>
@gregw gregw requested a review from janbartel April 28, 2021 08:38
janbartel
janbartel requested changes Apr 29, 2021
View reviewed changes
Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

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

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

jetty-webapp/src/test/java/org/eclipse/jetty/webapp/jsp/FakePrecompiledJSP.java Outdated
@@ -0,0 +1,37 @@
//
Copy link
Contributor

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.

jetty-webapp/src/test/java/org/eclipse/jetty/webapp/jsp/FakeJspServlet.java Outdated
@@ -0,0 +1,37 @@
//
Copy link
Contributor

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
update from review
2239740 
use inner classes

Signed-off-by: Greg Wilkins <[email protected]>
@gregw gregw requested a review from janbartel May 15, 2021 22:14
janbartel
janbartel approved these changes May 15, 2021
View reviewed changes
Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

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

LGTM

@gregw gregw merged commit edcaf70 into jetty-9.4.x May 16, 2021
@gregw gregw deleted the jetty-9.4.x-jsp-precompile-alt-servlet branch May 16, 2021 01:28
@gregw gregw linked an issue May 16, 2021 that may be closed by this pull request
Copy ServletHolder class/instance properly during startWebapp #6280
Closed
gregw pushed a commit that referenced this pull request May 16, 2021
@joakime @gregw
Copy ServletHolder class/instance properly during startWebapp (#6214)
47600f9 
 ServletHolder.copyClassServlet() method added to correctly copy held class.


Signed-off-by: Joakim Erdfelt <[email protected]>
Signed-off-by: Greg Wilkins <[email protected]>
@gregw gregw mentioned this pull request May 16, 2021
Merged
gregw added a commit that referenced this pull request May 16, 2021
@gregw @joakime
Jetty 10.0.x servletholder copy #6280 (#6281)
804630d 
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 <[email protected]>
Co-authored-by: Joakim Erdfelt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregw gregw gregw left review comments

@janbartel janbartel janbartel approved these changes

Assignees

@joakime joakime

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Copy ServletHolder class/instance properly during startWebapp
3 participants
@joakime @gregw @janbartel