fix: reuse ssl context in the sync client #222
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 testing, this reduces the openssl v3 performance penalty from ~200ms per connection to 9ms per connection. Original PR: openfga/sdk-generator#607
WalkthroughReplaces per-request TLS parameters with a single reusable SSLContext in the sync REST client and updates PoolManager/ProxyManager initialization to use it. Adds unit tests covering CA loading, client certs, verification toggling, proxy usage, and SSLContext reuse behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant RESTClient as RESTClientObject
participant SSL as ssl.create_default_context
participant C as SSLContext
participant Pool as urllib3.PoolManager
participant Proxy as urllib3.ProxyManager
App->>RESTClient: __init__(configuration)
RESTClient->>SSL: create_default_context(cafile=ssl_ca_cert)
SSL-->>RESTClient: C
alt cert_file provided
RESTClient->>C: load_cert_chain(cert_file, key_file)
end
alt verify_ssl is False
RESTClient->>C: check_hostname = False
RESTClient->>C: verify_mode = CERT_NONE
end
alt proxy configured
RESTClient->>Proxy: new(proxy_url, proxy_headers, ssl_context=C, maxsize=...)
else no proxy
RESTClient->>Pool: new(ssl_context=C, maxsize=...)
end
note over RESTClient,C: Reuse C for subsequent requests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 Pre-merge checks✅ Passed checks (3 passed)
|
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (70.73%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #222 +/- ##
==========================================
+ Coverage 70.70% 70.73% +0.03%
==========================================
Files 134 134
Lines 10881 10884 +3
==========================================
+ Hits 7693 7699 +6
+ Misses 3188 3185 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rhamzeh
force-pushed
the
fix/ssl-context-reuse
branch
from
0ad931b to
bb3acca
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openfga_sdk/sync/rest.py (1)
18-18: Bug: urllib.parse not imported (AttributeError risk when query_params is used)
urllib.parseisn’t imported;import urllibalone won’t expose theparsesubmodule.Apply:
-import urllib +from urllib.parse import urlencode- encoded_qs = urllib.parse.urlencode(query_params) + encoded_qs = urlencode(query_params)Also applies to: 287-290
🧹 Nitpick comments (4)
openfga_sdk/sync/rest.py (1)
512-514: Don’t clear the entire connection pool on every requestClearing the pool defeats HTTP/TLS connection reuse and undermines the perf win (you’ll still avoid context creation, but lose keep-alive/TLS session resumption). Release only the connection you used; keep the pool alive.
Apply:
- # Release the connection back to the pool - self.close() + # Release only this connection; keep pools for reuse + if _preload_content: + raw_response.release_conn()test/sync/rest_test.py (3)
538-541: Patch the symbols where they’re used (module-under-test) for robustnessPatching
urllib3.*andssl.create_default_contextat their global modules works, but patching via the module-under-test prevents interference from other imports.Apply representative changes (repeat for all occurrences):
-@patch('ssl.create_default_context') -@patch('urllib3.PoolManager') +@patch('openfga_sdk.sync.rest.ssl.create_default_context') +@patch('openfga_sdk.sync.rest.urllib3.PoolManager')-@patch('ssl.create_default_context') -@patch('urllib3.ProxyManager') +@patch('openfga_sdk.sync.rest.ssl.create_default_context') +@patch('openfga_sdk.sync.rest.urllib3.ProxyManager')Also applies to: 566-567, 597-599, 628-630, 664-665, 700-702
560-564: Use identity checks for the SSLContext mockCompare the exact context instance instead of equality (MagicMock equality is permissive).
Apply:
- assert call_kwargs['ssl_context'] == mock_ssl_context + assert call_kwargs['ssl_context'] is mock_ssl_contextAlso applies to: 592-595, 734-735
726-730: Make this assertion resilient (MagicMock truthiness is always True)
hasattr(...)/truthinesson MagicMock will pass even if the code modified the values. Seed explicit defaults and assert they remain unchanged.Apply:
- # Verify SSL verification settings were NOT modified (verify_ssl=True) - # check_hostname and verify_mode should remain at their default secure values - assert not hasattr(mock_ssl_context, 'check_hostname') or mock_ssl_context.check_hostname - assert not hasattr(mock_ssl_context, 'verify_mode') or mock_ssl_context.verify_mode != ssl.CERT_NONE + # Seed secure defaults and ensure they weren't changed + mock_ssl_context.check_hostname = True + mock_ssl_context.verify_mode = ssl.CERT_REQUIRED + RESTClientObject(configuration=mock_config) + assert mock_ssl_context.check_hostname is True + assert mock_ssl_context.verify_mode == ssl.CERT_REQUIRED
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openfga_sdk/sync/rest.py(3 hunks)test/sync/rest_test.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/sync/rest_test.py (2)
openfga_sdk/configuration.py (2)
timeout_millisec(731-735)timeout_millisec(738-742)openfga_sdk/sync/rest.py (2)
RESTClientObject(140-514)build_request(226-320)
🔇 Additional comments (2)
openfga_sdk/sync/rest.py (1)
158-169: Good move: single SSLContext reused and passed to Pool/Proxy managers
- Correct ordering when disabling verification (check_hostname before verify_mode).
- Passing the same context to both ProxyManager and PoolManager is the right alignment with urllib3 2.x.
Also applies to: 200-209, 213-218
test/sync/rest_test.py (1)
537-736: Nice coverage of SSLContext creation and reuseTests exercise CA loading, client certs, verify off, proxy path, and reuse behavior.
|
👍 My only note would be in the description. The ssl context creation difference is more like 30-40ms better per call, not 200ms better. |
|
Ah that was a typo - apologies. I wonder if one of the changes suggested by the AI bot above might help too: I didn't want to include that in this same PR as we have not tested that, so maybe Monday we'll push out a release with this ssl context fix, then we can have a follow up release with that change after testing. |
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 ~20ms per connection to 9ms per connection.
Original PR: openfga/sdk-generator#607 by @wadells
Description
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit
New Features
Tests
Refactor