Repository navigation
Raise the urllib3 pool maxsize to 10 and make it configurable - #1026
Open
jakobovski-arb wants to merge 1 commit into
Open
jakobovski-arb wants to merge 1 commit into
jakobovski-arb wants to merge 1 commit into
Conversation
jakobovski-arb
requested review from
jbonzo,
justinpolygon,
kschoche,
lukeoleson,
mmoghaddam385 and
suever
as code owners
July 31, 2026 22:45
jakobovski-arb
force-pushed
the
make-pool-maxsize-configurable
branch
2 times, most recently
from
July 31, 2026 22:51
431196b to
c19d1e2
Compare
BaseClient builds its PoolManager with num_pools but never maxsize, so urllib3's
default of ONE connection per host applies. num_pools caps the number of distinct
HOST pools, which does not help a client that talks to a single host from many
threads -- the two are often conflated, and passing a large num_pools looks like
it should raise concurrency but does nothing.
The effect under threads is a TLS handshake storm. Threads that lose the race for
the single pooled connection open their own, cannot return it ("Connection pool is
full, discarding connection"), so it is closed and the next request renegotiates
TLS. Measured against the trades endpoint over 240 calls:
single-threaded 0.0074 CPU-s per call, 0 pool-full warnings
20 threads 0.1309 CPU-s per call, 70 pool-full warnings (18x CPU)
and 40 threads were SLOWER than 20. With maxsize raised the warnings disappear,
CPU per call drops 2.9x, and throughput scales with threads again (25.7 -> 66.3
requests/sec in that benchmark).
Adds a `maxsize` parameter to RESTClient, threaded through to the PoolManager,
defaulting to 10.
The new default intentionally changes behaviour for every user, including those
who never touch the parameter -- that is the point of the change. maxsize=1 is not
a sensible ceiling for a client whose own constructor advertises pooling, and a
user hitting it has no way to discover why their threads are slow: the symptom
looks like a slow API, not a client setting. Fixing it only for people who find
the new argument would leave the common case broken.
10 matches both the existing num_pools default and requests' HTTPAdapter default,
so it is a conventional value rather than a tuned one. maxsize is a cap and not a
preallocation, so a single-threaded client still opens exactly one connection and
sees no difference; the change is felt only by concurrent callers, who are the
ones currently paying for it.
On BaseClient it is an optional keyword rather than a required one, so existing
subclasses that call super().__init__ keep working.
Tests cover the default, an explicit value, and that it is independent of
num_pools.
jakobovski-arb
force-pushed
the
make-pool-maxsize-configurable
branch
from
July 31, 2026 22:51
c19d1e2 to
dd4d88d
Compare
Contributor
Author
|
@justinpolygon please take a look |
justinpolygon
added a commit
that referenced
this pull request
Oct 16, 2026
Combines the community PRs that change how the REST client builds its urllib3 pool (#968, #1008, #1026, #1033, #1034). - CA bundle: both clients verify TLS with ca_certs if given, then the SSL_CERT_FILE or REQUESTS_CA_BUNDLE environment variable, then certifi (#998). - RESTClient(maxsize=10): connections kept per host. urllib3 defaults to 1, so threaded clients kept opening new TLS connections. - RESTClient(connection_pool_kw=...): other urllib3 pool options, merged over the defaults (#1009). - RESTClient(proxy=...): explicit proxy URL, with credentials from the URL. Proxy environment variables aren't read, so existing traffic doesn't change route. The WebSocket client already uses the system proxy via websockets. Co-authored-by: SACHU JAMES <142034811+SachuJames@users.noreply.github.com> Co-authored-by: Mohd Quamar Tyagi <104281681+Tyagiquamar@users.noreply.github.com> Co-authored-by: jakobovski-arb <201493996+jakobovski-arb@users.noreply.github.com> Co-authored-by: Roman Zydyk <31843161+romanzdk@users.noreply.github.com> Co-authored-by: Kevin Chiu <2712+kevinchiu@users.noreply.github.com>
justinpolygon
added a commit
that referenced
this pull request
Oct 16, 2026
Adds connection options to the clients, combining #968, #1008, #1026, #1033 and #1034 (contributors are credited as co-authors). - **Custom CA:** `ca_certs=` on `RESTClient` and `WebSocketClient`, defaulting to `SSL_CERT_FILE` / `REQUESTS_CA_BUNDLE`, then certifi. Closes #998. - **Pool size:** `RESTClient(maxsize=10)`. urllib3 defaults to 1 connection per host. - **Pool options:** `RESTClient(connection_pool_kw={...})`, merged over the defaults. Closes #1009. - **Proxy:** `RESTClient(proxy="http://user:pass@host:port")`. Proxy env vars aren't read, so nobody's traffic changes route. The README has a new **Network Configuration** section with examples for each case (custom CA, proxy, threads, other pool options). All new parameters are optional and added at the end of the signatures. The one behavior change: if `SSL_CERT_FILE` or `REQUESTS_CA_BUNDLE` points to a file, it's now used instead of certifi (as `requests` and Python do). If it points to a file that doesn't exist, it's skipped with a warning, so a stale value doesn't break anyone. Verified against the live API through a local TLS-inspecting proxy (mitmproxy with auth): REST and WebSocket connect with the proxy's CA (argument or env var) and fail certificate verification without it; wrong proxy credentials get 407. Under 16 threads, `maxsize=10` opened 16 TLS connections vs 57–70 with urllib3's default. --------- Co-authored-by: SACHU JAMES <142034811+SachuJames@users.noreply.github.com> Co-authored-by: Mohd Quamar Tyagi <104281681+Tyagiquamar@users.noreply.github.com> Co-authored-by: jakobovski-arb <201493996+jakobovski-arb@users.noreply.github.com> Co-authored-by: Roman Zydyk <31843161+romanzdk@users.noreply.github.com> Co-authored-by: Kevin Chiu <2712+kevinchiu@users.noreply.github.com>
This file contains hidden or 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
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.
Problem
BaseClientbuilds itsPoolManagerwithnum_poolsbut nevermaxsize, so urllib3's default of one connection per host applies:num_poolscaps the number of distinct host pools, which doesn't help a client that talks to a single host from many threads. The two are easy to conflate — passing a largenum_poolslooks like it should raise concurrency, but it changes nothing for a one-host client.Under threads the result is a TLS handshake storm: threads that lose the race for the single pooled connection open their own, then can't return it (
Connection pool is full, discarding connection), so it's closed and the next request renegotiates TLS.Measurements
Against the trades endpoint, 240 calls, same machine:
pool is fullwarningsAnd 40 threads were slower than 20 — the pool was the bottleneck, not the network. With
maxsizeraised: warnings gone, CPU per call down 2.9x, and throughput scales with threads again (25.7 → 66.3 req/sec in that benchmark).Change
Adds a
maxsizeparameter toRESTClient, threaded through to thePoolManager, defaulting to 10.The new default intentionally changes behaviour for every user, including those who never touch the parameter — that is the point of the change.
maxsize=1isn't a sensible ceiling for a client whose own constructor advertises pooling, and a user hitting it has no way to discover why their threads are slow: the symptom looks like a slow API, not a client setting. Fixing it only for people who go looking for a new argument would leave the common case broken.10 matches both the existing
num_poolsdefault andrequests'HTTPAdapterdefault, so it's a conventional value rather than a tuned one.maxsizeis a cap, not a preallocation — a single-threaded client still opens exactly one connection and sees no difference. The change is felt only by concurrent callers, who are the ones currently paying for it.On
BaseClientit's an optional keyword rather than a required one, so existing subclasses callingsuper().__init__(...)keep working.You might want to add a warning to existing users that num pools is being deprecated and users should use max size instead.
Five lines of production change; tests cover the default, an explicit value, and that it's independent of
num_pools.Checks
pytest test_rest test_websocket— 44 passed, no new failures. (test_clint_headers_concatalready fails on a cleanmasterfor me, unrelated to this change.)black 24.8.0 --checkclean on all three touched files.mypyclean on both changed modules.