feat: Enhance socket listener management and cleanup #4800
+985
−119
Conversation
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
- Introduced a new `release_socket_listener` function in Rust to manage socket reference counting and shutdown notifications. - Updated the Node.js client to call `ReleaseSocketConnection` when closing clients, ensuring proper cleanup of socket listeners. - Added tests to verify socket lifecycle contracts, ensuring sockets are removed upon client closure and do not leak across multiple client instances. - Enhanced Python client to release socket listeners on close, improving resource management. - Updated TypeScript configurations to include necessary types for better development experience. These changes improve the robustness of socket management across multiple language bindings, ensuring efficient resource cleanup and preventing socket leaks. Signed-off-by: Avi Fenesh <[email protected]>
- Enhanced IAM Auth tests to only run when explicitly enabled, improving CI/CD integration. - Updated multiple test files to include TypeScript type annotations for protocol parameters, enhancing type safety. - Refactored error handling in tests to use `throw new Error` instead of `fail`, aligning with best practices. - Added missing imports in test files to ensure proper functionality. These changes improve the robustness and maintainability of the test suite across the Node.js codebase. Signed-off-by: Avi Fenesh <[email protected]>
…de.js client - Updated the Rust socket listener to use sequential consistency for reference counting, enhancing thread safety. - Refactored the Node.js `releaseSocketListener` method to ensure local state is cleared even if an error occurs during socket release, improving robustness. - Enhanced error handling in the client close process to log errors without masking the original issues. - Updated tests to ensure proper error handling and socket management across different scenarios. These changes enhance the reliability and maintainability of socket management in the codebase. Signed-off-by: Avi Fenesh <[email protected]>
…iles - Deleted the `@types/eslint__js` entry from both the root and node-specific package.json files, as it is no longer needed. - This cleanup helps streamline the development dependencies and reduces potential confusion. Signed-off-by: [Your Name] <[email protected]> Signed-off-by: Avi Fenesh <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This pull request enhances socket listener management and cleanup across the GLIDE multi-language client library. The main purpose is to implement proper reference counting for socket listeners and ensure they are properly released when clients are closed, preventing socket file leaks and improving resource management.
Key changes implemented:
- Added a new
release_socket_listenerfunction in Rust core with reference counting and shutdown notification mechanisms - Updated Python and Node.js clients to call socket release functions during client cleanup
- Added comprehensive test suites to verify socket lifecycle contracts and prevent resource leaks
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| glide-core/src/socket_listener.rs | Major refactoring to implement reference counting, shutdown coordination, and proper socket cleanup with process-specific directories |
| python/glide-async/src/lib.rs | Exposes the new release_socket_listener_external function to Python |
| python/glide-async/python/glide/glide_client.py | Implements socket listener release during client closure with idempotent cleanup |
| node/rust-client/src/lib.rs | Exposes ReleaseSocketConnection function to Node.js |
| node/src/BaseClient.ts | Integrates socket path tracking and release calls during client lifecycle |
| python/tests/async_tests/test_socket_contracts.py | Comprehensive test suite for Python socket lifecycle contracts |
| node/tests/SocketContracts.test.ts | Comprehensive test suite for Node.js socket lifecycle contracts |
| node/tests/SharedTests.ts | Updates test patterns to use proper error throwing instead of fail() |
| Various TypeScript files | Minor improvements to type annotations and import patterns |
Co-authored-by: Copilot <[email protected]> Signed-off-by: Avi Fenesh <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Avi Fenesh <[email protected]>
… client - Enhanced error logging in the `BaseClient` class to maintain local state during socket cleanup, ensuring better error visibility. - Introduced a helper function `getSocketPath` to streamline socket path extraction from client objects, reducing repetitive type casting and improving code maintainability. - Updated tests to utilize the new helper function, enhancing clarity and consistency across socket lifecycle tests. These changes contribute to better error handling and cleaner code in the Node.js client implementation. Signed-off-by: Avi Fenesh <[email protected]>
…t management Signed-off-by: Avi Fenesh <[email protected]>
…clude from API functions Signed-off-by: Avi Fenesh <[email protected]>
ikolomi
requested changes
Sep 25, 2025
There was a problem hiding this comment.
Lets not mix these two issues as they are most probably orthogonal:
#4653 - errors during command processing
#4747 - potential leakage of the socket file names
Please split the PR to the 2 issues - UDS file pruning and command Invalid state errors.
Regarding UDS pruning - use unlink() after "let listener_socket = match UnixListener::bind(socket_path_cloned.clone())" - this should ensure the socket file is removed when process is closed, if that fails - add a signal handler (or atexit()) that calls remove_file. No need to touch the wrappers at all.
Regarding the Invalid state errors - IDK, i dont see how its related to the UDS socket. Need to reproduce
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
release_socket_listenerfunction in Rust to manage socket reference counting and shutdown notifications.ReleaseSocketConnectionwhen closing clients, ensuring proper cleanup of socket listeners.These changes improve the robustness of socket management across multiple language bindings, ensuring efficient resource cleanup and preventing socket leaks.
Issue link
This Pull Request is linked to issue #4747
#4653
Checklist
Before submitting the PR make sure the following are checked: