Retry the connection if a socket error is because the server went away. - #979
Conversation
StephenWakely
added
the
changelog/Changed
…empt A single extra send attempt only rescues a packet if the reconnect happens to land in the instant the peer comes back. For UDS SOCK_DGRAM, connect() succeeds immediately against a stale-but-still-present socket path even while nothing is listening, so the real failure only surfaces at send() time - meaning the one-shot retry almost never helps during a real outage (e.g. an agent restart). Retry submitting a packet with the same exponential backoff already used for the initial UDS connect, bounded by socket_connect_timeout, so it can actually ride out a longer outage.
|
_socket_lock only guarded socket creation/teardown (get_socket()/ close_socket()) and, for SOCK_STREAM, the send itself - the SOCK_DGRAM send and the socket-fetch that precedes it were unprotected. A thread calling close_socket() (e.g. after its own failed send) could close the fd while another thread already held a reference to it and was mid-send, raising EBADF and dropping that packet with no chance to retry. Widen _socket_lock to cover the whole fetch-and-send in _xmit_packet_attempt, and make it an RLock since that now nests with the lock already taken inside get_socket()/close_socket().
| except socket.timeout: | ||
| # dogstatsd is overflowing, drop the packets (mimics the UDP behaviour) | ||
| pass | ||
| return False |
There was a problem hiding this comment.
This return bypasses the SOCK_STREAM cleanup below, which is why test_stream_cleanup is failing in ci.
| self.socket_connect_timeout, | ||
| ) | ||
| break | ||
| time.sleep(min(backoff, remaining)) |
There was a problem hiding this comment.
Should we check the deadline after this sleep? Otherwise we might loop again even after expiration.
| log.debug( | ||
| "Connection error submitting packet: %s, reconnecting and retrying", socket_err | ||
| ) | ||
| self.close_socket() |
There was a problem hiding this comment.
Should self.close_socket() be called earlier before _socket_lock is released to avoid another sender using the failed socket?
| # getting a fresh full socket_connect_timeout allowance each time. | ||
| connect_timeout = self.socket_connect_timeout | ||
| if retry_deadline is not None: | ||
| connect_timeout = retry_deadline - time.time() |
There was a problem hiding this comment.
Should the timeout be calculated after acquiring the lock?
| def _xmit_packet(self, packet, is_telemetry): | ||
| # type: (str, bool) -> bool | ||
| retry_deadline = None | ||
| if self.socket_connect_timeout and self.socket_connect_timeout > 0: |
There was a problem hiding this comment.
Do we need to gate this to be for UDS only?
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
The merge request has been interrupted because the build 0 took longer than expected. The current limit for the base branch 'master' is 120 minutes. Possible reasons:
|
StephenWakely
added
ci/integrations
3 participants
What does this PR do?
Retries reconnecting and resending a payload on send errors socket error.
Description of the Change
On transient send errors -
errno.ECONNREFUSED,errno.ECONNRESET,errno.ENOTCONN,errno.EPIPE,errno.ENOENT, close the socket and attempt to resend. We only attempt resending once to avoid getting stuck in a loop if the agent doesn't come back beyond thesocket_connect_timeout.Alternate Designs
Possible Drawbacks
Verification Process
Additional Notes
Release Notes
Review checklist (to be filled by reviewers)
changelog/label attached. If applicable it should have thebackward-incompatiblelabel attached.do-not-merge/label attached.kind/andseverity/labels attached at least.