fix: avoid globally monkeypatching Requests - #1256
Conversation
Give each Edge HSM signing mechanism its own Unix-socket session so importing the SDK no longer replaces process-wide Requests helpers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Carter Tinney (cartertinney)
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
The persistent session lacks deterministic cleanup, and the import-time regression is not directly tested.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Replaces global Requests monkeypatching with per-instance Unix-socket sessions for Edge HSM communication.
Changes:
- Creates a private
requests_unixsocket.Session. - Routes trust-bundle and signing requests through that session.
- Updates unit-test mocks for session-based requests.
File summaries
| File | Description |
|---|---|
edge_hsm.py |
Introduces and uses the private session. |
test_edge_hsm.py |
Updates request mocks and tests session creation. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Close private Unix-socket sessions during sync and async client shutdown and on Edge client construction failures. Add direct regression coverage ensuring module import never invokes the global Requests monkeypatch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use context-managed Unix-socket sessions for each trust-bundle and signing request so cleanup does not depend on the client shutdown lifecycle. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Follow-up: revised the cleanup design in be77e2d. Each Edge HSM operation now owns a context-managed Unix-socket Session, so cleanup is deterministic even if the SDK client is never shut down. The client-lifecycle ownership plumbing added in the prior review fix has been removed. |
There was a problem hiding this comment.
🟢 Approval recommended
The scoped-session implementation is correct, preserves existing request behavior, and has focused regression coverage.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
/azp run Python E2E |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run Python E2E |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Document why the import regression test must resolve classes through the reloaded module. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
requests_unixsocket.monkeypatch()callTesting
uv run --frozen pytest tests/unit/iothub/test_edge_hsm.py tests/unit/iothub/test_sync_clients.py tests/unit/iothub/aio/test_async_inbox.py tests/unit/iothub/aio/test_async_clients.py tests/unit/iothub/test_inbox_manager.py -W error::RuntimeWarning -q(1564 passed, 6 skipped)requests_unixsocket.monkeypatch()is not called