fix(python): reuse ssl context in the python sync client - #607
Conversation
This brings ssl context handling in line with the async client. Importantly, openssl has a pretty signifigant performance regression in creating ssl contexts v3.0+ that is mitigated by paying the context creation tax once, instead of for every request. Based on my testing, this reduces the openssl v3 performance penalty from ~200ms per connection to 9ms per connection.
wadells
changed the title
wadells
changed the title
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughReworks RESTClientObject TLS setup to build and reuse an SSLContext, loading CA and client certs, optionally disabling verification. Passes the SSLContext to urllib3 PoolManager/ProxyManager, removing explicit cert_reqs. Adds a comment referencing OpenSSL 3.0+ performance considerations. Public API remains unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Config as Configuration
participant REST as RESTClientObject.__init__
participant SSL as ssl.SSLContext
participant U3 as urllib3 (Pool/Proxy Manager)
User->>REST: instantiate with Configuration
REST->>Config: read ssl_ca_cert, cert_file, key_file, verify_ssl, proxies
REST->>SSL: create_default_context(cafile)
alt client cert provided
REST->>SSL: load_cert_chain(cert_file, keyfile)
end
alt verify_ssl == false
REST->>SSL: set check_hostname = False, verify_mode = CERT_NONE
end
alt proxy configured
REST->>U3: ProxyManager(proxy_url, ssl_context=SSL, ca_certs, cert, key)
else no proxy
REST->>U3: PoolManager(ssl_context=SSL, ca_certs, cert, key)
end
REST-->>User: client ready (API unchanged)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
config/clients/python/template/src/sync/rest.py.mustache (3)
506-508: Don't clear the pool per request; release the connection instead.Calling self.close() nukes the pool and defeats connection reuse, undermining this PR’s goal. Release the connection only when we’ve preloaded the content; leave raw responses to callers.
- # Release the connection back to the pool - self.close() + # Release the connection back to the pool when preloaded + if _preload_content: + raw_response.release_conn()
257-268: Fix isinstance() unions — current code raises at runtime.isinstance(timeout_val, float | int) is invalid; use a tuple. Same for tuple detection stays as-is.
- if isinstance(timeout_val, float | int): + if isinstance(timeout_val, (float, int)): if timeout_val > 100: timeout_val /= 1000 timeout = urllib3.Timeout(total=timeout_val) elif isinstance(timeout_val, tuple) and len(timeout_val) == 2:
302-304: Same isinstance() fix for body type check.- elif isinstance(body, str | bytes): + elif isinstance(body, (str, bytes)): args["body"] = body
🧹 Nitpick comments (2)
config/clients/python/template/src/sync/rest.py.mustache (2)
282-285: Encode repeated query params correctly.urlencode(..., doseq=True) preserves repeated keys and list values.
- if query_params: - encoded_qs = urllib.parse.urlencode(query_params) + if query_params: + encoded_qs = urllib.parse.urlencode(query_params, doseq=True) args["url"] = f"{url}?{encoded_qs}"
45-49: Docstring param name mismatch.The docstring refers to :param resp: but the parameter is response.
- :param resp: The urllib3.HTTPResponse object. + :param response: The urllib3.HTTPResponse object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
config/clients/python/template/src/sync/rest.py.mustache(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
config/**/*.mustache
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Validate mustache syntax and variable references across all template files, including CHANGELOG.md.mustache
Files:
config/clients/python/template/src/sync/rest.py.mustache
config/**/*.{json,mustache}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never hardcode API keys or credentials in configuration or template files
Files:
config/clients/python/template/src/sync/rest.py.mustache
When ssl_context is provided, urllib3 uses it and ignores per-arg TLS settings. Passing both is redundant.
|
Thanks for the PR, @wadells! While my Python competency is definitely less than that of @evansims, some thoughts from a higher level:
To unblock you, short-term options if you need this now while we collaborate on the HTTP client injection:
If you're open to it, we can collaborate to pivot this PR toward adding support for passing a custom HTTP client and use that to handle SSL context reuse cleanly. What do you think? |
|
@rhamzeh: Thanks for the reply! Overall, I'd characterize my response as: "Don't let perfect get in the way of better." Priority wise this is causing (admittedly relatively unnoticeable to humans) performance papercuts for hundreds of thousands of Zapier customers. Probably 10 hours of human time wasted per day, in 30-40ms slices.
I agree -- this is a much cleaner architecture, and I would have loved to have this tuning knob when I came across this ssl perf issue. However:
This is major scope creep. If this a route Okta would like to go, I propose that you all make these enhancements after the performance issue is no longer a concern. I'd like to point out my patch to the sync client is identical to how ssl configuration is currently handled in the async client. This a design choice @adriantam and you made in #22. This isn't a new pattern for the codebase, this is fixing a performance diff between the two clients.
I'm puzzled by this remark. Could you perhaps rephrase? I don't add or expose ssl_context it in the openfga-sdk API. This patch doesn't change any public signatures or configuration options.
Alternatively, consider a release addressing performance issues as 0.9.6 (it is a bugfix after all) and we can change the public facing API as needed in v0.10.0.
As discussed above, I'd rather have a separate patch for (potentially breaking) API changes, and keep this one focused on a pretty clear cut ssl instantiation performance difference between the async and the sync client. |
|
@wadells - that makes sense. There's some minor things that we can tackle in follow-ups. Would you mind signing the Linux Foundation's CLA here: #607 (comment) so we can merge? It's enforced by the CNCF that it be signed before we can get this merged. |
For anything that doesn't touch the public api or also require refactoring the async client, I'd be happy to address it in this PR. For those bigger changes, lets handle them as follow ups.
Unfortunately, I'm not authorized sign this on behalf of Zapier. I reached out to my legal team (who does have the power to sign on behalf of the company) yesterday and they've been assessing. I'll poke them again now. I could probably cheat around it and sign as an individual, but these changes were developed as part of my job so the ethical thing is to wait for Zapier legal. |
|
@rhamzeh: CLA signed! |
|
@wadells thanks! Will aim to get this merged an in the Python SDK by tomorrow. Thanks for the investigation and the fix ❤️ |
|
Thanks @wadells! |
rhamzeh
changed the title
This brings ssl context handling in line with the async client. Importantly, openssl has a pretty signifigant performance regression in creating ssl contexts v3.0+ that is mitigated by paying the context creation tax once, instead of for every request. Based on testing, this reduces the openssl v3 performance penalty from ~200ms per connection to 9ms per connection. Original PR: openfga/sdk-generator#607
* fix: reuse ssl context in the sync client This brings ssl context handling in line with the async client. Importantly, openssl has a pretty signifigant performance regression in creating ssl contexts v3.0+ that is mitigated by paying the context creation tax once, instead of for every request. Based on testing, this reduces the openssl v3 performance penalty from ~200ms per connection to 9ms per connection. Original PR: openfga/sdk-generator#607 * chore: add tests for ssl context reuse --------- Co-authored-by: Walt Della <walt.della@zapier.com>
3 participants
Description
This brings sync client ssl context handling in line with the async client. Importantly, openssl has a pretty significant performance regression in creating ssl contexts v3.0+ that can be mitigated by paying the context creation tax once, instead of for every request.
What problem is being solved?
Performance penalty observed when we upgraded from debian bullseye (openssl 1.1.1w) to debian bookworm (openssl 3.0.17). We saw a roughly 40ms performance penalty across our use of the sync client.
How is it being solved?
We remove an expensive call (ssl context creation) from each request, and instead do it once, upon client instantiation. This matches the behavior in the aio client.
What changes are made to solve it?
See above
Testing
TLDR: On the worst openssl version I tested, we saw a 40ms reduction in p50 FGA call time. This accounts for 75% of the total call time. 4x performance improvement!
I didn't write any tests on the FGA side. However I did do benchmarking using the following script.
fgabench.py
Here are the results tested on an amd64 t2.medium AWS instance in us-west-2.
openfga-sdkis the performance as of v0.9.4sdk-patchedis v0.9.4 with a monkey patchedopenfga_sdk.sync.rest.RESTClientObject.__init__to use my new versionurllibis the defaulturllib.request.urlopen("https://api.us1.fga.dev/")behavior. Included as a reference point.urllib+ctxisurllibwith a reused ssl-context. Same fix, but without any of the potential complications of FGA client or server logic.References
See openssl/openssl#17064 for a comprehensive discussion of the openssl performance issues.
Review Checklist
mainSummary by CodeRabbit