Conversation
Bus adapter changes
There was a problem hiding this comment.
Pull Request Overview
This PR introduces IPv6 socket compatibility improvements, enhanced error handling for closed sockets, updated documentation, and bumped the version to 2.0.0rc2. The main focus is on making socket operations more robust across different network configurations.
- IPv6 compatibility: Updated
getsockname()usage to handle both IPv4 (2-tuple) and IPv6 (4-tuple) return values - Error handling: Added try-except blocks around
getpeername()calls to gracefully handle closed sockets - Documentation: Expanded README with installation instructions, dependencies, and development guidelines
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/protocol_proxy/proxy/asyncio.py | Updated socket parameter extraction to support IPv6 by taking only the first two elements from getsockname() |
| src/protocol_proxy/ipc/asyncio.py | Applied the same IPv6 compatibility fix for inbound server socket parameters |
| src/protocol_proxy/ipc/gevent.py | Added error handling for getpeername() calls to prevent crashes when sockets are closed, though several string formatting issues were introduced |
| pyproject.toml | Bumped version to 2.0.0rc2 and fixed mypy python_version to be a string |
| README.md | Comprehensive documentation update with badges, installation instructions, and disclaimer, but contains two typos |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| peer_name = f'to {s.getpeername()}' | ||
| except OSError: | ||
| peer_name = '' | ||
| _log.warning(f'Outbound socket to {peer_name} was ready, but no outbound message was found.') |
There was a problem hiding this comment.
String formatting issue: The log message is missing "socket" between "Outbound" and the peer name. It should be f'Outbound socket {peer_name} was ready, but no outbound message was found.' When peer_name is empty, it produces "Outbound socket to was ready" which is grammatically incorrect, and when it's populated, it produces "Outbound socket to to [address]" with duplicate "to".
| peer_name = f'to {s.getpeername()}' | |
| except OSError: | |
| peer_name = '' | |
| _log.warning(f'Outbound socket to {peer_name} was ready, but no outbound message was found.') | |
| peer_name = s.getpeername() | |
| except OSError: | |
| peer_name = None | |
| if peer_name: | |
| _log.warning(f'Outbound socket {peer_name} was ready, but no outbound message was found.') | |
| else: | |
| _log.warning('Outbound socket was ready, but no outbound message was found.') |
| _log.warning(f'{self.proxy_name}: Received unknown method name: {headers.method_name}' | ||
| f' from {s.getpeername()} with request ID: {headers.request_id}') | ||
| f' {peer_name} with request ID: {headers.request_id}') |
There was a problem hiding this comment.
String formatting issue: The log message has an extra space before {peer_name}. It should be f'{self.proxy_name}: Received unknown method name: {headers.method_name}{peer_name} with request ID: {headers.request_id}' (without the space before {peer_name}). When peer_name is empty, it produces an extra space, and when it's populated with from [address], it already has a leading space.
| if len(received) == 0: | ||
| _log.warning(f'{self.proxy_name} received closed socket from ({s.getpeername()}.') | ||
| try: | ||
| peer_name = f' from {s.getpeername()}.' |
There was a problem hiding this comment.
String formatting issue: When getpeername() succeeds, peer_name is set to f' from {s.getpeername()}.' (with a trailing period), but the log message is f'{self.proxy_name} received closed socket {peer_name}' which will produce a period in the middle of the sentence when the peer name is available. The period should be removed from the peer_name assignment on line 150, or added to line 153 after {peer_name}.
| peer_name = f' from {s.getpeername()}.' | |
| peer_name = f' from {s.getpeername()}' |
| raise NotImplementedError(f'Unknown protocol version ({version_num})' | ||
| f' received from: {s.getpeername()}') | ||
| try: | ||
| peer_name = f' received from: {s.getpeername()}.' |
There was a problem hiding this comment.
String formatting issue: When getpeername() succeeds, peer_name is set to f' received from: {s.getpeername()}.' (with a trailing period), but this creates double periods in the error message. The period should be removed from the peer_name assignment on line 158, or the message should be structured differently to avoid duplication.
| peer_name = f' received from: {s.getpeername()}.' | |
| peer_name = f' received from: {s.getpeername()}' |
| peer_name = f': {s.getpeername()}.' | ||
| except OSError: | ||
| peer_name = '.' | ||
| _log.warning(f'{self.proxy_name}: Unable to read headers from socket: {peer_name}') |
There was a problem hiding this comment.
String formatting issue: When getpeername() succeeds, peer_name is set to f': {s.getpeername()}.' (with a leading colon and trailing period), but the log message already has : before it, creating double colons: "Unable to read headers from socket: : [address].". The leading colon should be removed from the peer_name assignment on line 213.
| peer_name = f': {s.getpeername()}.' | |
| except OSError: | |
| peer_name = '.' | |
| _log.warning(f'{self.proxy_name}: Unable to read headers from socket: {peer_name}') | |
| peer_name = f' {s.getpeername()}' | |
| except OSError: | |
| peer_name = '' | |
| _log.warning(f'{self.proxy_name}: Unable to read headers from socket:{peer_name}') |
| [](https://github.com/eclipse-volttron/lib-protocol-proxy/actions/workflows/run-tests.yml) | ||
| [](https://pypi.org/project/protocol-proxy/) | ||
|
|
||
| This library provides the user with the ability to automatically deploy and manager proxy processes for handling |
There was a problem hiding this comment.
Typo in "manager": "the ability to automatically deploy and manager proxy processes" should be "the ability to automatically deploy and manage proxy processes".
| This library provides the user with the ability to automatically deploy and manager proxy processes for handling | |
| This library provides the user with the ability to automatically deploy and manage proxy processes for handling |
| ``` | ||
|
|
||
| Protocol Proxy plugins should include "protocol-proxy" as a requirement, so users of existing | ||
| plugins are encouraged to instead install the plugin for that pacakge directly. |
There was a problem hiding this comment.
Typo in "pacakge": "install the plugin for that pacakge directly" should be "install the plugin for that package directly".
| plugins are encouraged to instead install the plugin for that pacakge directly. | |
| plugins are encouraged to instead install the plugin for that package directly. |
|
Commits in this PR are included in another PR. They will be merged there. |
2 participants
This pull request introduces several improvements and fixes across the codebase, focusing on enhanced documentation, improved socket handling for IPv6 compatibility, and better error handling in socket operations. The most significant changes are grouped below:
Documentation and Packaging:
README.mdwith a clearer project description, installation instructions, dependency details, development guidelines, and disclaimer. Added badges for Python versions, test status, and PyPI version.2.0.0rc1to2.0.0rc2inpyproject.toml.python_versionfield in the mypy configuration to be a string for consistency.Socket Handling and Compatibility:
getsockname()in bothsrc/protocol_proxy/ipc/asyncio.pyandsrc/protocol_proxy/proxy/asyncio.pyto only use the first two elements (host,port), ensuring compatibility with IPv6 sockets which return a 4-tuple. [1] [2]Error Handling Improvements:
src/protocol_proxy/ipc/gevent.pyby wrappinggetpeername()calls in try/except blocks to gracefully handle cases where the socket is closed and avoid raisingOSError. This change affects logging and exception messages throughout the file for better robustness. [1] [2] [3]