Skip to content
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

Handle ECONNREFUSED and typo fix #113

Merged
merged 2 commits into from
Aug 5, 2019
Merged

Conversation

redhotpenguin
Copy link
Contributor

This PR adds error handling for ECONNREFUSED, and also fixes a small typo. This allows requests that fail due to a previous request encountering ECONNREFUSED to retry if the service on 8125 is available. Small edge case.

@KSerrania KSerrania self-requested a review July 3, 2019 15:12
Copy link
Contributor

@KSerrania KSerrania left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

Could you add a test case for this scenario, akin to the one we have for ENOTCONN here?

@@ -45,6 +45,7 @@ def write(message)
# Try once to reconnect if the socket has been closed
retries ||= 1
if retries <= 1 && boom.is_a?(Errno::ENOTCONN) or
retries <= 1 && boom.is_a?(Errno::ECONNREFUSED) or
retries <= 1 && boom.is_a?(IOError) && boom.message =~ /closed stream/i
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: now that we have three cases, we could refactor this condition like that:

if retries <= 1 &&
   (boom.is_a?(Errno::ENOTCONN) or 
    boom.is_a?(Errno::ECONNREFUSED) or 
    boom.is_a?(IOError) && boom.message =~ /closed stream/i)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing - added refactor and unit test. Thanks for your patience, was offline last couple weeks.

Oddly I was seeing these test failures - not sure if it's my environment:

  1) Failure:
Datadog::Statsd::GC#test_0001_produces low amounts of garbage for simple methods [/Users/fmoyer/Code/dogstatsd-ruby/spec/statsd_spec.rb:1047]:
                      sourcefile                         sourceline  class   count
-------------------------------------------------------  ----------  ------  -----
/Users/fmoyer/Code/dogstatsd-ruby/spec/helper.rb                 23  Array       1
/Users/fmoyer/Code/dogstatsd-ruby/lib/datadog/statsd.rb          88  Array       1
/Users/fmoyer/Code/dogstatsd-ruby/lib/datadog/statsd.rb         578  String      1
/Users/fmoyer/Code/dogstatsd-ruby/lib/datadog/statsd.rb         571  String      1
/Users/fmoyer/Code/dogstatsd-ruby/lib/datadog/statsd.rb         568  String      1.
Expected: 6
  Actual: 5

  2) Failure:
Datadog::Statsd::GC#test_0002_produces low amounts of garbage for timing [/Users/fmoyer/Code/dogstatsd-ruby/spec/statsd_spec.rb:1051]:
                      sourcefile                         sourceline  class   count
-------------------------------------------------------  ----------  ------  -----
/Users/fmoyer/Code/dogstatsd-ruby/spec/helper.rb                 23  Array       1
/Users/fmoyer/Code/dogstatsd-ruby/lib/datadog/statsd.rb          88  Array       1
/Users/fmoyer/Code/dogstatsd-ruby/lib/datadog/statsd.rb         578  String      1
/Users/fmoyer/Code/dogstatsd-ruby/lib/datadog/statsd.rb         571  String      1
/Users/fmoyer/Code/dogstatsd-ruby/lib/datadog/statsd.rb         568  String      1.
Expected: 6
  Actual: 5

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is strange, seems like there's one allocation that's not done on line 87 (88 in your output).
The CI tests passed, and I tested locally your fork with success, so that may be due to a specific environment.
I'll keep this in mind, but it shouldn't block this PR.
If the problem persists on your environment, could you create an issue so that we can keep track of it?

Copy link
Contributor

@KSerrania KSerrania left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

@redhotpenguin
Copy link
Contributor Author

Sure thing, feel free to merge when ready.

@KSerrania KSerrania merged commit 9964f72 into DataDog:master Aug 5, 2019
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.

2 participants