Jetty 11 support #1942

reta · 2022-09-03T21:52:16Z

Jetty 11 support, closes #1760

Finish upload refactoring
- multipart/mixed is not handled yet (4 tests in the MultipartBodyMatchingAcceptanceTest)
Fix logging support (slf4j 2.0.0): switched to Logback 1.4.0 since Log4j2 does not support Slf4j 2.0 APIs (yet)
HttpServletResponse::setStatus(int sc, String sm) is deprecated and not implemented by Jetty 11 (StubbingAcceptanceTest::settingStatusMessage)
Fix failing test cases: 1 test case is failling
- GeneralCodingRulesTest > RULE_NO_CLASSES_SHOULD_THROW_GENERIC_EXCEPTIONS FAILED

tomakehurst

Thanks for making a start on this.

Please let me know if you'd like any help getting specific features working again. I know the faults stuff can be tricky as I've had to re-fix them at least twice during Jetty upgrades.

tomakehurst · 2022-09-04T13:40:26Z

src/main/java/com/github/tomakehurst/wiremock/jetty/JettyHttpServerFactory.java

@@ -34,23 +34,15 @@ private static Constructor<? extends JettyHttpServer> getServerConstructor() {
    try {
      Class<? extends JettyHttpServer> serverClass =
          (Class<? extends JettyHttpServer>)
-              Class.forName("com.github.tomakehurst.wiremock.jetty94.Jetty94HttpServer");
+              Class.forName("com.github.tomakehurst.wiremock.jetty11.Jetty11HttpServer");


I'm wondering if we should ditch the reflection now and just construct it directly.

This was originally used so that both JDK7/Jetty 9.2.x and JDK8+/Jetty 9.4.x could be selected appropriately at runtime. Since we're dropping support for all Jetty versions except 11 (for now at least) there's no good reason I can think of to load reflectively.

It'll help cold start performance too if we do it directly.

Sure, I think direct usage will be better option

Addressed that as well, thanks @tomakehurst

tomakehurst · 2022-09-04T13:41:33Z

src/test/java/com/github/tomakehurst/wiremock/jetty11/JettyHttpServerTest.java

@@ -82,7 +84,7 @@ public void testStopTimeout() {

  @Test
  public void testStopTimeoutNotSet() {
-    long expectedStopTimeout = 1000L;
+    long expectedStopTimeout = 5000L;


Does the original 1s value cause problems with this Jetty verrsion?

Yes, this is very interesting but the graceful shutdown in Jetty 11 is taking more time, ~3s on average, I set it to 5s to eliminate possible flakiness.

Wow, 3s is pretty bad. Since many (most probably) WireMock tests start and stop the server per test case, this is going hurt performance pretty badly.

I wonder if there's something we do to tune this (or something we're doing to cause in in the first place)?

Yeah, we surely could look at it, I have not spend much time into looking"why" but Graceful.shutdown is the one taking time, we could look which components here to look after

reta · 2022-09-04T13:59:33Z

Thanks for making a start on this.

Please let me know if you'd like any help getting specific features working again. I know the faults stuff can be tricky as I've had to re-fix them at least twice during Jetty upgrades.

I will, thanks @tomakehurst , I will try to fix what I could before asking for your time.

tomakehurst · 2022-09-09T13:21:55Z

I've just pushed a bunch of fixes to the master branch to get Jetty 9.4.48 working, in case that's any use.

reta · 2022-09-09T13:30:53Z

👍 thanks @tomakehurst , I will rebase shortly, almost done with multipart/*

reta · 2022-09-09T17:00:27Z

@tomakehurst I think we are there, could I ask you please to run the workflow (if possible) to check if I missed something ~~, I have only 1 test case failing: GeneralCodingRulesTest > RULE_NO_CLASSES_SHOULD_THROW_GENERIC_EXCEPTIONS FAILED~~. Thank you.

reta · 2022-09-09T17:06:36Z

src/main/java/com/github/tomakehurst/wiremock/http/multipart/FileUpload.java

+ * properly. To preserve backward compatibility and support wider range of multipart content,
+ * re-implementing this part of the upload.
+ */
+class FileUpload {


@tomakehurst this is probably the most controversial change but here is the story. Jetty supports multipart/form-data out of the box, so that is cool. However, the fileupload components also support multipart/mixed and multipart/related (+ attachments). The problems with fileupload are: a) no releases for the last 4 years b) tight integration with javax.servlet.

There are 3 routes to take here:

drop multipart/mixed and multipart/related support, not very pleasant for backward compatibility

adapt fileupload to deal with Jakarta Servlet (this pull request)

drop fileupload and write own multipart parsers for multipart/mixed, multipart/related and attachments (may take some time)

Appreciate your opinion on the matter, thanks!

I think dropping support for /mixed and /related wouldn't go down well, since it only got added relatively recently: #1415.

The second option seems like the best choice to me, since we know the library does what we need. Rolling your own always takes time and inevitably means fixing bugs for a while afterwards.

Do you see any downsides of option 2?

@tomakehurst thanks for opinion, the only downside I see is bringing the additional code (which I shamelessly copied, acknowledging that) which would need to be maintained, otherwise - reasonable tradeoff.

The other consideration is licensing. It'd be the only case where we're copying source code into the project rather than depending on libraries and I'm not sure what impact this has on our ability to present WireMock as being from a single copyright source etc.

The license is Apache v2.0 [1], should be good on this front, and there are still chances that fileupload gets new release with Jakarta support [2], no hints when though ...

[1] https://www.apache.org/licenses/LICENSE-2.0
[2] https://github.com/apache/commons-fileupload/tree/master/src/main/java/org/apache/commons/fileupload2/jaksrvlt

Another solution to this that might be worth considering is adding just the servlet API back into the dependencies.

I haven't fully tested it yet, but adding implementation 'javax.servlet:servlet-api:2.5' to the build file seems to fix the issue and allow you to use the fileupload library.

Technically you are right (since the Servlet API is referenced only in some static methods if I am not mistaken), but that would be quite confusing to be fair. It may work for wiremock specifically but have unintended consequences for the projects using it: I would imaging classpath scanning issues fe (even if we are talking about tests only).

It would certainly complicate things for users who've already got the servlet API in their classpath from somewhere else, and it'd be a shame to make people switch to the standalone JAR just to avoid this.

So I guess the copied version is the least worst option at the moment.

There are some things that are necessary to do in order to comply with the Apache 2.0 licence, which are summarised here:
https://tldrlegal.com/license/apache-license-2.0-(apache-2.0)
The NOTICE file we'll need to include is here: https://github.com/apache/commons-fileupload/blob/commons-fileupload-1.4/NOTICE.txt

@tomakehurst thanks a lot, I will do that shorly (travelling next couple of days), apologies for that

@tomakehurst NOTICE.txt is added, https://github.com/wiremock/wiremock/pull/1942/files#diff-b810ebe4ffda4293c79d292059b45b59de7e649e86314ffd5a1ee4eb4b1e08c8, thanks!

tomakehurst · 2022-09-19T16:11:59Z

Can I suggest dropping Java 8 from the build matrix (since our minimum is now 11)?

tomakehurst · 2022-09-27T13:35:41Z

Excellent progress - great to see all the tests green.

Do you have any theories on how we can deal with this slow shutdown issue?

reta · 2022-09-27T13:37:18Z

Excellent progress - great to see all the tests green.

Thank you!

Do you have any theories on how we can deal with this slow shutdown issue?

Oh, sorry, I forget to troubleshoot it, I will update you on the matter shortly

reta · 2022-09-30T01:02:05Z

Do you have any theories on how we can deal with this slow shutdown issue?

@tomakehurst getting back to this one, so the Graceful shutdown includes 6 components (see please below), each of them is subject of own Graceful shutdown sequences. The NetworkTrafficServerConnector (last one in the list) takes indeed more than 1 second - it shuts all connectors / acceptors (we have in this case 4 of them, vs 2 in case of Jetty 9), it probably adds up.

Elapsed for NetworkTrafficServerConnector@18cc679e{HTTP/1.1, (http/1.1, h2c)}{localhost:0}: 1ms
Elapsed for o.e.j.s.ServletContextHandler@19058533{/__admin,null,SHUTDOWN}: 0ms
Elapsed for o.e.j.s.ServletContextHandler@4315e9af{/,null,SHUTDOWN}: 0ms
Elapsed for HTTP2SessionContainer@730e5763[size=0]: 1ms
Elapsed for HTTP2SessionContainer@7275c74b[size=0]: 0ms
Elapsed for NetworkTrafficServerConnector@263f04ca{SSL, (ssl, alpn, h2, http/1.1)}{localhost:0}: 1017ms

I think we could safely drop 5s to 3s but going below could be a bit risky (unstable).
Wdyt?

tomakehurst · 2022-09-30T13:19:32Z

I think there's something fundamentally wrong if we can't get down into the sub 100ms range. Certainly, we'll be sacrificing a lot of performance vs. the previous version.

Are you able to profile it to see what the connector class is spending so much time on?

reta · 2022-09-30T13:46:55Z

Are you able to profile it to see what the connector class is spending so much time on?

No, I instrumented the server code to find the cultprit but didn't profile, I am afraid that rabbit hole could take me a while, I may look into that in a few weeks, apologies but do not have enough spare time these days.

Signed-off-by: Andriy Redko <drreta@gmail.com>

…ustom status message Signed-off-by: Andriy Redko <drreta@gmail.com>

Signed-off-by: Andriy Redko <drreta@gmail.com>

…ate Jetty11HttpServer (no reflection) Signed-off-by: Andriy Redko <drreta@gmail.com>

Signed-off-by: Andriy Redko <drreta@gmail.com>

reta · 2022-10-08T20:03:10Z

Are you able to profile it to see what the connector class is spending so much time on?

@tomakehurst I was able to get back to that, so there is another timeout in play here, shutdownIdleTimeout with default set to 1 second. I lowered the default to 100ms and also exposed it as part of JettySettings (so it becomes possible to customize it by the need), the stop delay is reverted back to 1 second, no issues observed (at least locally).

jimma · 2022-10-18T12:09:26Z

@reta @tomakehurst It's nice to see the Jetty11 support is addressed in this PR. Nice work ! @tomakehurst Is there any plan to have a release with this change soon ?

reta · 2022-11-04T01:21:45Z

@tomakehurst anything else left or I could help you with to move this thing forward? It would be very helpful, thank you.

tomakehurst · 2022-11-04T11:56:19Z

I think this is looking pretty good, although I wanted to do a bit of profiling on it before merging as it still seems to be a bit slower than than with Jetty 9.x.

(Was also waiting until I'd shipped the last minor release to avoid having to deal with branches, but that's done now).

raphw · 2022-11-25T11:14:57Z

No intend to nag, I was just wondering if you have a release date in mind. The current wiremock is incompatible with Spring Boot 3 and due to the Jakarta switch, there's quite a bit of work involved that grows stale quickly through the huge change set, that's the reason for my interest. Thanks for the great work!

tomakehurst · 2022-11-25T11:43:43Z

@raphw I'll try to look at this the w/c 12th of December (I'm maxed out with the day job up until then)

raphw · 2022-11-25T11:55:05Z

Brilliant. I'll try to set off some time myself for performance testing the diff!

marcingrzejszczak · 2022-11-25T13:24:28Z

Why is WireMock incompatible with Boot 3? Spring Cloud Contract works really well with it 🤔

reta · 2022-11-25T13:38:22Z

Why is WireMock incompatible with Boot 3? Spring Cloud Contract works really well with it 🤔

At the moment, WireMock uses Jetty 9 (and javax.servlet.* under the hood), while Spring Boot 3 moved to Jetty 11 and JakartaEE [1]. I would clarify that it is not compatible with Spring Boot 3 web applications that use Jetty 11, you may overcome some of these limitations using wiremock-standalone as a dependency.

[1] https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-3.0-Release-Notes

marcingrzejszczak · 2022-11-25T13:40:17Z

That's exactly what we're doing - using wiremock-standalone - I guess that's why we don't face any issues.

jorgerod · 2022-12-21T17:50:30Z

We have the same problem. We have a framework that we want to migrate to SpringBoot 3 and we are blocked by this issue.

@tomakehurst Are you planning to release this feature soon?

Thank you very much

reta · 2022-12-30T18:31:08Z

@tomakehurst thank you!

tomakehurst · 2022-12-30T19:08:57Z

@reta thank you for contributing this!

reta force-pushed the issue-1760 branch 2 times, most recently from 923c30c to ffeee5a Compare September 4, 2022 01:33

tomakehurst requested changes Sep 4, 2022

View reviewed changes

reta force-pushed the issue-1760 branch from 0027de7 to 7a82c46 Compare September 6, 2022 01:10

reta force-pushed the issue-1760 branch 3 times, most recently from 1eed022 to e320d83 Compare September 9, 2022 16:55

reta commented Sep 9, 2022

View reviewed changes

tompahoward mentioned this pull request Sep 11, 2022

wiremock-jre8-standalone affected by high severity CVE due to jetty 9.4.46.v20220331 #1944

Closed

reta force-pushed the issue-1760 branch from e320d83 to 9e8b998 Compare September 18, 2022 23:50

reta marked this pull request as ready for review September 19, 2022 16:45

reta changed the title ~~[WIP] Jetty 11 support~~ Jetty 11 support Sep 19, 2022

reta added 8 commits October 8, 2022 15:57

Jetty 11 support

db9454c

Signed-off-by: Andriy Redko <drreta@gmail.com>

Finished upload refactoring

4bdf0a6

Signed-off-by: Andriy Redko <drreta@gmail.com>

Added explicit handling of HTTP CONNECT (in case of proxy)

ce8a9fa

Signed-off-by: Andriy Redko <drreta@gmail.com>

Using org.eclipse.jetty.server.Response::setStatusWithReason to set c…

f01a01e

…ustom status message Signed-off-by: Andriy Redko <drreta@gmail.com>

Fixing GeneralCodingRulesTest test case failures

68b963a

Signed-off-by: Andriy Redko <drreta@gmail.com>

Dropping JDK-8 from build / test CI actions

8e8606c

Signed-off-by: Andriy Redko <drreta@gmail.com>

Simplifying JettyHttpServerFactory to directly reference and instanti…

6a6fe10

…ate Jetty11HttpServer (no reflection) Signed-off-by: Andriy Redko <drreta@gmail.com>

Added NOTICE.txt

adedc2d

Signed-off-by: Andriy Redko <drreta@gmail.com>

reta force-pushed the issue-1760 branch from 56bb3b4 to 6a5599f Compare October 8, 2022 19:59

--signoff

84eedcf

reta force-pushed the issue-1760 branch from 6a5599f to 84eedcf Compare October 8, 2022 21:33

rasklaad mentioned this pull request Oct 12, 2022

Are there any plans to upgrade com.github.tomakehurst:wiremock-jre8 to Jetty 10? #1980

Open

Merge branch 'master' into issue-1760

01c2106

sp00m mentioned this pull request Nov 28, 2022

Spring Boot 3 qudini/qudini-reactive#166

Closed

Merge branch 'master' into issue-1760

86f64d4

tomakehurst merged commit 86f64d4 into wiremock:master Dec 30, 2022

erikgb mentioned this pull request Jan 19, 2023

Upgrade Jetty 9 to 10/11/12 eclipse-rdf4j/rdf4j#4252

Open

Jetty 11 support #1942

Jetty 11 support #1942

Conversation

reta commented Sep 3, 2022 • edited

tomakehurst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reta Sep 4, 2022 • edited

Choose a reason for hiding this comment

reta commented Sep 4, 2022

tomakehurst commented Sep 9, 2022

reta commented Sep 9, 2022

reta commented Sep 9, 2022 • edited

reta Sep 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reta Sep 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomakehurst commented Sep 19, 2022

tomakehurst commented Sep 27, 2022

reta commented Sep 27, 2022

reta commented Sep 30, 2022

tomakehurst commented Sep 30, 2022

reta commented Sep 30, 2022

reta commented Oct 8, 2022 • edited

jimma commented Oct 18, 2022

reta commented Nov 4, 2022

tomakehurst commented Nov 4, 2022

raphw commented Nov 25, 2022 • edited

tomakehurst commented Nov 25, 2022

raphw commented Nov 25, 2022

marcingrzejszczak commented Nov 25, 2022

reta commented Nov 25, 2022

marcingrzejszczak commented Nov 25, 2022

jorgerod commented Dec 21, 2022

reta commented Dec 30, 2022

tomakehurst commented Dec 30, 2022

reta commented Sep 3, 2022 •

edited

reta Sep 4, 2022 •

edited

reta commented Sep 9, 2022 •

edited

reta Sep 9, 2022 •

edited

reta Sep 20, 2022 •

edited

reta commented Oct 8, 2022 •

edited

raphw commented Nov 25, 2022 •

edited