Visitar URL original
Copy ServletHolder class/instance properly during startWebapp by joakime · Pull Request #6214 · jetty/jetty.project · GitHub
Skip to content

Copy ServletHolder class/instance properly during startWebapp - #6214

Merged
gregw merged 10 commits into
jetty-9.4.xfrom
jetty-9.4.x-jsp-precompile-alt-servlet
May 16, 2021
Merged

Copy ServletHolder class/instance properly during startWebapp#6214
gregw merged 10 commits into
jetty-9.4.xfrom
jetty-9.4.x-jsp-precompile-alt-servlet

Conversation

@joakime

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

Copy link
Copy Markdown
Contributor

joakime added 5 commits April 21, 2021 10:11
@joakime
Attempt to replicate bug reported against precompiled jsps in descrip…
ca82dde 
…tor.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Speculative ServletHolder.copyClassServlet() method added.
0032484 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Adding System/Server ClasspathPattern fix
4424bfc 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@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 <joakim.erdfelt@gmail.com>
@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 <joakim.erdfelt@gmail.com>
@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

@gregw gregw left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread jetty-webapp/src/test/java/org/eclipse/jetty/webapp/ForcedServletTest.java Resolved
Comment thread jetty-webapp/src/test/java/org/eclipse/jetty/webapp/ForcedServletTest.java Resolved
Comment thread jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java Resolved
janbartel
janbartel requested changes Apr 27, 2021
View reviewed changes
Comment thread jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java Resolved
Comment thread jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java Resolved
Comment thread jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java Resolved
Comment thread jetty-webapp/src/test/resources/alt-jsp-webdefault.xml Resolved
@gregw
Copy just the class
6df2cf2 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@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 <gregw@webtide.com>
janbartel
janbartel requested changes Apr 28, 2021
View reviewed changes
Comment thread jetty-webapp/src/test/java/org/eclipse/jetty/webapp/jsp/FakeJspServlet.java Resolved
@gregw
Simplified test
c29cb55 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
gregw requested a review from janbartel April 28, 2021 08:38
@joakime
Minor cleanup to reduce noisy parts of test
f8f8be6 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
janbartel
janbartel requested changes Apr 29, 2021
View reviewed changes

@janbartel janbartel left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread jetty-webapp/src/test/java/org/eclipse/jetty/webapp/jsp/FakePrecompiledJSP.java Outdated
@@ -0,0 +1,37 @@
//

@janbartel janbartel Apr 29, 2021

Copy link
Copy Markdown
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.

Comment thread jetty-webapp/src/test/java/org/eclipse/jetty/webapp/jsp/FakeJspServlet.java Outdated
@@ -0,0 +1,37 @@
//

@janbartel janbartel Apr 29, 2021

Copy link
Copy Markdown
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 <gregw@webtide.com>
@gregw
gregw requested a review from janbartel May 15, 2021 22:14
janbartel
janbartel approved these changes May 15, 2021
View reviewed changes

@janbartel janbartel left a comment

Copy link
Copy Markdown
Contributor

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 <joakim.erdfelt@gmail.com>
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@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 <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

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