Skip to content

[dogstatsd] sock.setblocking(0) for UDP socket#590

Merged
prognant merged 2 commits intomasterfrom
prognant/dogstatsd-fix-potentially-blocking-situation
Jul 22, 2020
Merged

[dogstatsd] sock.setblocking(0) for UDP socket#590
prognant merged 2 commits intomasterfrom
prognant/dogstatsd-fix-potentially-blocking-situation

Conversation

@prognant
Copy link
Copy Markdown
Contributor

@prognant prognant commented Jul 15, 2020

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)

  • Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
  • PR should not have do-not-merge/ label attached.
  • If Applicable, issue must have kind/ and severity/ labels attached at least.

@prognant prognant requested a review from a team as a code owner July 15, 2020 13:19
self.socket = sock
else:
sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
sock.setblocking(0)
Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor Author

@prognant prognant Jul 15, 2020

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.

@prognant prognant requested a review from jd July 15, 2020 19:41
Copy link
Copy Markdown
Member

@olivielpeau olivielpeau left a 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 👍

@prognant
Copy link
Copy Markdown
Contributor Author

prognant commented Jul 22, 2020

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 EAGAIN are already properly caught, so we should be covered.

@olivielpeau
Copy link
Copy Markdown
Member

ok great! Thanks for confirming

@prognant prognant merged commit 2dbe2e4 into master Jul 22, 2020
@prognant prognant deleted the prognant/dogstatsd-fix-potentially-blocking-situation branch July 22, 2020 15:04
@zippolyte zippolyte added the changelog/Added Added features results into a minor version bump label Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/Added Added features results into a minor version bump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants