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

Only fetch checksums for final gradle releases during build #2917

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mfussenegger
Copy link
Contributor

@mfussenegger mfussenegger commented Oct 20, 2023

This is a follow up to #2775
I frequently experience build failures due to connection errors.

After some debugging with wireshark, netstat and
-Djdk.httpclient.HttpClient.log=all I suspect that I'm getting rate
limited.

Somewhere between 150 and 200 requests, the connections either get
closed by the remote or almost stall.

To mitigate the issue, this excludes nightly, snapshot, rc and broken
gradle releases. The biggest impact has cutting out RC releases:

total versions: 386
without nightly: 385
without RCs: 173
without broken: 172

Note that this still doesn't completely fix the issue for me, but it
helps reduce the chance for it happening.

I also raised an issue in the gradle repo: gradle/gradle#26793 (I doubt they'll address this soon, if at all, but it's worth a shot)

This is a follow up to eclipse-jdtls#2775
I frequently experience build failures due to connection errors.

After some debugging with wireshark, netstat and
`-Djdk.httpclient.HttpClient.log=all` I suspect that I'm getting rate
throttled.

Somewhere between 150 and 200 requests, the connections either get
closed by the remote or almost stall.

To mitigate the issue, this excludes nightly, snapshot, rc and broken
gradle releases. The biggest impact has cutting out RC releases:

total versions: 386
without nightly: 385
without RCs: 173
without broken: 172

Note that this still doesn't completely fix the issue for me, but it
helps reduce the chance for it happening.
Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

@testforstephen , let me know your thoughts on this.

I don't think I like this approach as it reduces what versions of Gradle we would support where supporting them all requires just some build-time processing. Users might still be interest in running the nightlies/snapshots/rcs for the latest upcoming release so it might be nice to include those.

Even so, I feel like including all those versions should continue to be done as long as the build machine it's running on doesn't have these rate limiting issues. Would you consider including a flag to only build the releases ? Or sorting the entries to do the releases first ?

If having this task run inconsistently every time is the issue, why not run it once (on some non-limited machine, or just compile the full list), and then use -Declipse.jdt.ls.skipGradleChecksums to skip the generation entirely until needed again ?

return
}
HttpRequest request = HttpRequest.newBuilder().uri(URI.create(it.wrapperChecksumUrl)).build()
futures.add(client.sendAsync(request, BodyHandlers.ofString()).thenApply { response ->
if (response.statusCode() == 301) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you eliminating redirection support because none of the links currently require it ? Wouldn't it be good to have this just in case ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The client does that implicitly if configured to:

HttpClient client = HttpClient.newBuilder()
	.followRedirects(Redirect.NORMAL)
	.build();

@mfussenegger
Copy link
Contributor Author

I suspected this might be a bit controversial. Part of my reasoning was that people using pre-release versions should kinda know what they're doing and should be fine with accepting the unknown checksum.

Using -Declipse.jdt.ls.skipGradleChecksums works for me, but others could run into the build failures too - making this a hurdle to contributions. It looks like the files are behind a cloudflare CDN, which could make this a region dependant issue.

@rgrunber
Copy link
Contributor

rgrunber commented Oct 20, 2023

I'm open to this change. It definitely improves the code by handling the redirects. It's just a question of whether we have some guard for enable/disable nightly/snapshot/rcs at build-time or just get rid of them.

@jdneo
Copy link
Contributor

jdneo commented Oct 22, 2023

Or what about introduce a flag to disable parallel downloading? Since it looks like the parallel downloading works fine in the CI machines.

@testforstephen
Copy link
Contributor

Somewhere between 150 and 200 requests, the connections either get
closed by the remote or almost stall.

@mfussenegger Is the rate limit per minute, hour or day? What is the use case that requires you to build the jdt.ls from code frequently?

I don't think I like this approach as it reduces what versions of Gradle we would support where supporting them all requires just some build-time processing. Users might still be interest in running the nightlies/snapshots/rcs for the latest upcoming release so it might be nice to include those.

I have similar concern about excluding some gradle version explicitly, this definitely has impact on end-user experience.

@mfussenegger
Copy link
Contributor Author

mfussenegger commented Oct 23, 2023

Is the rate limit per minute, hour or day? What is the use case that requires you to build the jdt.ls from code frequently?

Probably per minute. I get it sometimes on the first build, after not having built for several days or even weeks.
I don't build it that frequently. Maybe once a week or less. Mostly to ensure changes haven't broken anything in nvim-jdtls and if they do, that I can address them before a new eclipse.jdt.ls release.

If the RC releases are really such a concern I don't mind closing this and create a new PR for the .followRedirects(Redirect.NORMAL) change. That's unrelated anyway.

I ended up adding -Declipse.jdt.ls.skipGradleChecksums for me, but I still think that this is a potential stepping stone for contributors. Although given that the ~170 downloads are sometimes still too much, maybe another option would be to default to sequential download, and have a flag to opt-in to the concurrent download?

That would ensure people who check this out for the first time have a reliable build, and regulars should know how to tune it.

@rgrunber
Copy link
Contributor

rgrunber commented Oct 23, 2023

@mfussenegger , I'm ok with switching to sequential builds (by default) again as long as we have a way to re-enable the parallel workflow (via. system property) in client-side setups like https://github.com/redhat-developer/vscode-java/blob/master/gulpfile.js#L163 . Is that possible with the current code ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants