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

Fix set_default_connection_limit not being respected #1053

Merged
merged 2 commits into from May 19, 2023

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented May 19, 2023

With all the default global connection pool shuffling that has happened, we missed making sure set_default_connection_limit! was still being respected. It was updating the global connection limit ref, but the global pools were still stuck with whatever their values were when first initialized.

This PR reinitializes the global connection pools when the default connection pool limit is changed.

With all the default global connection pool shuffling that has
happened, we missed making sure `set_default_connection_limit!`
was still being respected. It was updating the global connection
limit ref, but the global pools were still stuck with whatever their
values were when first initialized.

This PR reinitializes the global connection pools when the default
connection pool limit is changed.
test/client.jl Outdated
@@ -13,6 +13,7 @@ using InteractiveUtils: @which

# test we can adjust default_connection_limit
HTTP.set_default_connection_limit!(12)
@test HTTP.Connections.TCP_POOL[].max == 12
Copy link

Choose a reason for hiding this comment

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

Possibly paranoid, but might save us at some point: Perhaps test the same expectation of the other pools?

Copy link

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

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

Naively LGTM, assuming no cleanup need be done after rebinding the pools? Thanks Jacob!

test/client.jl Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented May 19, 2023

Codecov Report

Merging #1053 (ad18bcc) into master (d8392a2) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1053      +/-   ##
==========================================
+ Coverage   85.25%   85.27%   +0.02%     
==========================================
  Files          32       32              
  Lines        3031     3036       +5     
==========================================
+ Hits         2584     2589       +5     
  Misses        447      447              
Impacted Files Coverage Δ
src/Connections.jl 84.58% <100.00%> (+0.28%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Co-authored-by: Nick Robinson <npr251@gmail.com>
@quinnj quinnj merged commit 1070b46 into master May 19, 2023
12 checks passed
@quinnj quinnj deleted the jq-fix-default-connection-limit-changing branch May 19, 2023 17:13
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.

None yet

4 participants