Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Closed the original PR (#521) as I felt it would be cleaner this way. This PR only includes the changes to streams and clients and incorporates the feedback from #521. The docker based testing harness I used to stress test all the different network components is removed from this pr but can still be found in my personal fork here: https://github.com/cjjacks/AIT-Core/tree/issue-517-docker. The stress can be run by checking out that branch and running Additionally, I merged master into this branch to incorporate the linting and formatting changes. All subsequent commits after the merge were done with the pre-commit hooks installed. Let me know if there is any additional feedback. This time I will rebase and clean up the commit history after feedback so I dont end having to trash the PR if serious changes are needed. |
|
1 similar comment
|
| self.pub = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | ||
|
|
||
| def publish(self, msg): | ||
| self.pub.connect(self.addr_spec) |
There was a problem hiding this comment.
Just checking if the socket.connect() should be called per-message? Or would it be better to call it once in the constructor?
Also, should re-connection attempt be added in case of broken connection?
| if type(output) is int: | ||
| self.addr_spec = ("localhost", output) | ||
| elif utils.is_valid_address_spec(output): | ||
| protocol, hostname, port = output.split(":") |
There was a problem hiding this comment.
Since we have is_valid_address_spec() , could you please add a parse_address_spec() which replaces string.split(":")
| raise (ValueError("TCPInputClient: Invalid Specification")) | ||
|
|
||
| def __exit__(self, exc_type, exc_val, exc_tb): | ||
| try: |
There was a problem hiding this comment.
please consolidate into a single method which is called by exit's and del, and consider Niling the fields after they are closed/killed
| if stream_type == "inbound": | ||
| strm = self._create_inbound_stream(s["stream"]) | ||
| if type(strm) == PortInputStream: | ||
| if ( |
There was a problem hiding this comment.
Could this check be made into a static function in streams.py, something like is_server_stream(strm) ?
| ): | ||
| raise ValueError(f"Input stream specification invalid: {parsed_inputs}") | ||
|
|
||
| # backwards compatability with original UDP server spec |
There was a problem hiding this comment.
More a question than request, the output factory warns when discarding excess outputs. Should a similar warning be included here for the non-ZMQStream cases when only first input of many is used?
| .. _Stream_config: | ||
|
|
||
| TCP/UDP Address Specification: | ||
|
|
There was a problem hiding this comment.
Per the doc updates above, could a note be added that states, for backward compatibility, integer ports are treated as 'udp:localhost:port'
2 participants
Creating a new pull request for #517 without the formatting, linting and docker changes. Additionally incorporating feedback on address specifications