-
Notifications
You must be signed in to change notification settings - Fork 305
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
[dogstatsd] sock.setblocking(0) for UDP socket #590
[dogstatsd] sock.setblocking(0) for UDP socket #590
Conversation
@@ -246,6 +246,7 @@ def get_socket(self): | |||
self.socket = sock | |||
else: | |||
sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) | |||
sock.setblocking(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order is different compared to the block above, where the blocking is set after connect
.
I think your order is fine here, I'm more worried about the one above. But they should probably not be different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I harmonised both cases, I did some test to ensure it does not break anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the effect of making the UDP socket non-blocking is that on a send
, if the socket send buffer is full, an exception will be raised instead of waiting (blocking) for the send buffer to not be full.
Are we catching these errors correctly in UDP mode if they to be raised by a send
? If so, I think this change is fine 👍
In theory I agree on the effect of the call, as far as I checked cpython it should be consistent across platforms including windows. Regarding the error, exceptions with errno set to |
ok great! Thanks for confirming |
What does this PR do?
Add sock.setblocking(0) for UDP socket when connecting to a dogstastd server.
It resolve a blocking situation that was happening with internal test, making half test job stuck and ultimately failing after hitting the 1h timeout.
Description of the Change
cf. above.
Alternate Designs
N/A
Possible Drawbacks
None identified so far.
Verification Process
Manual tests.
Test pipelines are all green with this change.
Additional Notes
IPv6 seems unsupported for dogstatsd communication, should be fixed.
Release Notes
cf. PR title
Review checklist (to be filled by reviewers)
changelog/
label attached. If applicable it should have thebackward-incompatible
label attached.do-not-merge/
label attached.kind/
andseverity/
labels attached at least.