Skip to content

Fix concurrent socket connection issue #44

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 12, 2020

Conversation

jaredcnance
Copy link
Member

@jaredcnance jaredcnance commented Jul 12, 2020

Closes #40

This makes the TCP client safe to use across threads by allowing only a single thread to connect at a time. Additionally, it adds a single connect+retry for messages that fail because of a disconnected peer. Note that there is currently an issue with the test_can_recover_from_agent_shutdown that is a result of the test socket not being properly cleaned between shutdown and resume. This causes the test agent to fail to start occasionally.

@jaredcnance jaredcnance force-pushed the jared/#40-tcp-socket-errors branch from 6f1ab0d to e5a88c7 Compare July 12, 2020 15:26
@jaredcnance jaredcnance merged commit b873620 into master Jul 12, 2020
@jaredcnance jaredcnance deleted the jared/#40-tcp-socket-errors branch July 12, 2020 15:33
@hussam789
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix potential infinite recursion

The retry counter is initialized to 1 but never decremented in the initial
connection attempt. If the first connection fails, you'll still have retry=1
when recursively calling send_message, potentially leading to infinite recursion
if connection keeps failing.

aws_embedded_metrics/sinks/tcp_client.py [61-67]

 def send_message(self, message: bytes, retry: int = 1) -> None:
     if retry < 0:
         log.error("Max retries exhausted, dropping message")
         return
 
     if self._sock is None or self._sock._closed or self._should_connect:  # type: ignore
         self.connect()
+        if self._should_connect:  # If connection failed
+            return self.send_message(message, retry - 1)
  • Apply this suggestion
Suggestion importance[1-10]: 8

__

Why: The suggestion identifies a potential infinite recursion bug by not decrementing the retry counter when the initial connection fails and proposes a change that handles this case correctly.

Medium
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TCP Socket errors - Transport endpoint is already connected
2 participants