SK-2521-Refactor file handling and error management in service accoun… #230
Conversation
…t and vault controllers
There was a problem hiding this comment.
Pull request overview
This PR addresses resource leak issues by ensuring proper cleanup of HTTP connections and file handles throughout the codebase. The changes implement try-finally blocks to guarantee that responses and files are closed after use, preventing potential memory and connection leaks.
Changes:
- Added try-finally blocks to ensure HTTP response objects are properly closed after processing in vault operations
- Refactored file handling in service account utilities to use context managers (with statements) for automatic resource cleanup
- Restructured connection handling to ensure both response and session objects are properly closed
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| skyflow/vault/controller/_vault.py | Added try-finally blocks around response processing for insert, detokenize, and upload_file operations to ensure api_response.close() is called |
| skyflow/vault/controller/_detect.py | Added response cleanup in polling, file deidentification, and detect run retrieval; implemented proper file closing logic for opened file paths |
| skyflow/vault/controller/_connections.py | Restructured exception handling to ensure both HTTP response and session are properly closed |
| skyflow/service_account/_utils.py | Refactored credential file handling from manual open/close to context managers and improved exception handling specificity |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aadarsh-st
force-pushed
the
aadarsh-st/SK-2521-Fix-python-issue
branch
from
aa913e9 to
f6d3f8b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aadarsh-st
changed the base branch from
main
to
himanshu/2496-update-linting-rules-and-fix-hardcoded-values-in-python-v2-sdk
aadarsh-st
force-pushed
the
aadarsh-st/SK-2521-Fix-python-issue
branch
from
c37ecfa to
b635a1e
Compare
…es-and-fix-hardcoded-values-in-python-v2-sdk' into aadarsh-st/SK-2521-Fix-python-issue
saileshwar-skyflow
left a comment
saileshwar-skyflow
left a comment
There was a problem hiding this comment.
In the Vault controller, the API response is closed only for insert, detokenize, and upload operations. Could you please check the other interfaces as well?
The same applies to the Detect controller.
It's done |
aadarsh-st
merged commit a7de2f0
into
himanshu/2496-update-linting-rules-and-fix-hardcoded-values-in-python-v2-sdk
Fixed the below issue:
Readers and HTTP connections are not closed after receiving responses, which may lead to resource leaks.