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

[JENKINS-72016] Define a thread pool for ProxyConfiguration’s HttpClient #8490

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

jglick
Copy link
Member

@jglick jglick commented Sep 14, 2023

#7398 advertises a standardized HttpClient factory, and says

The returned instances of HttpClient […] are intended to be short-lived, not to be stored in memory in some sort of cache […].

When this advice is followed in some CloudBees CI plugins, obtaining a fresh HttpClient per request, under some circumstances where many requests are made the result is a gigantic number of threads with HttpClient in the name, eventually crashing the controller.

See JENKINS-72016.

Testing done

Ran the new test with

watch 'jstack $(jps -lm | fgrep booter | awk "{print \$1}")|egrep \^\"|wc -l'

Prior to patch: the test slows down and finally hangs without making it to 9000 requests. There are in the vicinity of 18000 threads in the JVM at this point. Sometimes the JVM crashes with OutOfMemoryError claiming it cannot create a new native thread. (This is pre-Loom!)

After patch: the test runs steadily to completion. The JVM thread count fluctuates around 4000 or so. A new HttpClientImpl.SelectorManager thread is created each time, but these seem to terminate themselves fairly soon rather than piling up indefinitely.

Proposed changelog entries

  • Do not create a large number of threads when making numerous HTTP requests.

Proposed upgrade guideliness

N/A

Before the changes are marked as ready-for-merge:

Maintainer checklist

@jglick jglick changed the title Define a thread pool for ProxyConfiguration’s HttpClient [JENKINS-72016] Define a thread pool for ProxyConfiguration’s HttpClient Sep 14, 2023
@jglick jglick requested a review from Vlatombe September 14, 2023 17:14

@Test
public void httpClientExecutor() throws Exception {
for (int i = 0; i < 50_000; i++) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Takes <1m on my laptop, so I am hoping this is reasonable to keep as is. If not, it could be @Ignored or deleted.

Copy link
Member

@Vlatombe Vlatombe left a comment

Choose a reason for hiding this comment

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

Looks reasonable and fixes the performance issue when using a lot of throw-away HttpClients

Copy link
Member

@Vlatombe Vlatombe left a comment

Choose a reason for hiding this comment

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

Looks reasonable and fixes the performance issue when using a lot of throw-away HttpClients

@Vlatombe Vlatombe requested a review from a team September 14, 2023 18:33
@Vlatombe Vlatombe added the bug For changelog: Minor bug. Will be listed after features label Sep 14, 2023
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Sep 14, 2023
@timja timja removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Sep 14, 2023
@timja
Copy link
Member

timja commented Sep 14, 2023

(removed ready for merge, the new test is failing on windows)

@timja timja added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Sep 14, 2023
@NotMyFault NotMyFault merged commit 3e1747e into jenkinsci:master Sep 20, 2023
16 checks passed
@jglick jglick deleted the httpClientExecutor branch September 20, 2023 21:16
krisstern pushed a commit to krisstern/jenkins that referenced this pull request Oct 2, 2023
…Client` (jenkinsci#8490)

* Define a thread pool for `ProxyConfiguration`’s `HttpClient`

* Skip test on Windows jenkinsci#8490 (comment)

(cherry picked from commit 3e1747e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For changelog: Minor bug. Will be listed after features ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants