Actually use TLS in (some) Maven tests and shove a few minutes from their execution #4591
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rewrite-maven/src/test/java/org/openrewrite/maven/internal/MavenPomDownloaderTest.java
Outdated
Show resolved
Hide resolved
d84fb1e to
6a2eb13
Compare
|
Oh wow, thanks for the very detailed analysis and fix! Should be a great quality of life improvement to shave this much of the build time, even with tests in parallel. I also like the additional suggestions with regards to removing the dependency on people.apache.org, as indeed I don't think that's necessary and seems wasteful. As to blocking http traffic at all; I'm not sure we can already make that switch, as we have a long tail of older projects to support still, especially in enterprises, and even have recipes to coach those away from using http repositories. I'm similarly on the fence about not trying https when a port is specified, just because outdated or misconfiguration is to be expected at scale, but perhaps others like @pstreef can chime in here as well. I'm tempted to merge this already as I've come to trust your quality work, but as I'm away for the weekend and reviewing from mobile it's probably best to have a last look on my laptop by Monday before I merge. Either way thanks a lot already, really stand out work these improvements. |
|
First: this is awesome! Thanks so much for this contribution and the detailed explanation. Indeed the As Tim said, we need to keep http support. The nature of the project is to upgrade code, so we need to support many older configurations to be able to handle as much old code as possible. And http is still used a lot. I too am unable to review the code, but I read the description and am very happy with your results. Thanks again! |
My idea would be to disable it by default, and have a configuration flag to re-enable it. That would be quite simple, even though it wouldn’t actually match what Maven does (and I don’t know about Gradle). Also remember that a lot of people are using internal mirrors anyways, which may or may not be HTTPS, but it’s quite independent from the age of the project to migrate. I would assume people are progressively migrating to HTTPS (Maven 3.8.1 was released more than 3 years ago), so disabling it by default should be ok. Take your time for the review, especially since I reorganized |
BTW people.apache.org replies today (but of course there is no repository there any more) |
|
FYI, I was curious about why This is just by adding System.out.println(request.getMethod() + " " + request.getUrl());to FYI, I dived into Maven’s integration tests recently, and they actually run mostly offline. They just have a |
|
That indeed sounds worthwhile. Maybe we could sum up the total time spent downloading POMs (and jars? And Metadata?) during an entire test execution just to get an idea of how much time could be saved here. Maybe we could configure the tests to use a read-through local file system cache that gets persisted somewhere, so that it can be reused for subsequent runs and in the CI be cached (and then periodically be deleted?). |
Reduces what needs to be resolved to speed up test.
|
Did a first pass through (still from crappy airport wifi), but it looks good. I've tried to swap out the pom used in I do like the option of blocking http:// by default, perhaps through |
|
Thanks once again @DidierLoiseau ! I'll go ahead and merge this iteration already, and we can iterate on some of the other proposals. Great that you noticed and explored this test performance degradation, and appreciate the potential other improvements pointed out like blocking http:// by default. |
|
Following the merge Gradle Develocity's list of slowest test is indeed looking a lot better:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's changed?
I enabled TLS support in OkHttp MockWebServer for some tests, in order to be able to reply with an actual HTTPS response.
On my machine,
MavenPomDownloaderTestgoes down from 1m15 to 18s, andMavenParserTestgoes from 7min to 1min! (more on this minute below)This can also serve as POC to be used in other tests.
What's your motivation?
Some time ago, I got curious about what was making those tests so slow, so investigated them. I had kept this on the side while working on other things but I thought it was still worth integrating.
Without TLS support, the
MavenPomDownloaderfirst tries HTTPS before HTTP. As MockWebServer does not expect this, it tries to parse the TLS handshake as an HTTP request line, and since it does not receive a\n, it just waits for it until the client times out, and then it logs an error:See also the GitHub Actions log and search for occurrences of
MavenParserTestto find occurrences of the above error. You’ll notice they are all frommirrorsAndAuth(). Now check the timestamps! 😏By enabling HTTPS, we can avoid this timeout. As the client times out after 10s and retries multiple times, this can spare a lot of time.
Anything in particular you'd like reviewers to focus on?
MavenPomDownloaderTestI reorganized this test class by grouping most tests between two
@Nestedclasses: with and without TLS. The remaining 18s are due mostly to tests actually relying on the timeout/fallback on HTTP, or contacting online servers.Side note: I did this a few weeks ago, I now noticed a small change in behavior from
normalizeRepository(), since, I guess, #4506: it does not seem to setknownToExisttotrueafter validating the URL. I don’t know if this was intended. (My originalusesHttpsWhenAvailable()test was expecting it, now I removed that)MavenParserTestI only changed
mirrorsAndAuth()which goes down from 6min to ~300ms. I actually didn’t remember it was originally so slow, the same test in this build from mid-september seems to have taken ~1min. I did my changes around Sep 24th, commit d2c825c, I still have my old branch if you are interestedI also didn’t remember about the remaining 1 minute, but I think something has changed in between… and it’s not OpenRewrite!
The
prerequisite()test now takes a varying 1-2minutes alone! And this test does… nothing! It’s just a pom without parent and a single dependency:org.apache.maven.reporting:maven-reporting-api:3.0.But that dependency has a transitive dependency:
org.apache.maven.doxia:doxia-sink-api:1.0. If you follow from parent to parent, you end up withorg.apache:apache:4(from 2007 😅) and it declares a single repository: http://people.apache.org/repo/m2-snapshot-repositoryAt the moment I am writing these lines, HTTPS seems available on that domain (and HTTP even redirects to it), but requests time out. Indeed, since September 12th, they have shut down the service.
Maybe a different dependency should be used for this test?
Anyone you would like to review specifically?
@timtebeek
Have you considered any alternatives or workarounds?
The TLS setup could probably be centralized somewhere but I don’t think it is worth it until it gets used more broadly. Maybe a way to disable the HTTPS attempt in tests could be good.
Any additional context
The strategy of trying HTTPS first is actually inconsistent with Maven’s modern behavior. Since Maven 3.8.1, in order to fix CVE-2021-26291, Maven completely blocks HTTP repositories by default. I think OpenRewrite should do the same.
Moreover, OpenRewrite does not check if a port is specified in the URL (like with MockWebServer). If a port is specified in an HTTP URL, it does not make sense at all to try HTTPS on the same port! This was what was causing trouble with MockWebServer in the first place…
Checklist