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

Support a :raise_timeout_errors option to raise timeouts as Resolv::ResolvError #14

Merged
merged 1 commit into from
Nov 25, 2023

Conversation

jeremyevans
Copy link
Contributor

This allows to differentiate a timeout from an NXDOMAIN response.

Fixes [Bug #18151]

…esolvError

This allows to differentiate a timeout from an NXDOMAIN response.

Fixes [Bug #18151]
@gmcabrita
Copy link

Seems this was forgotten, any chance this can get merged?

gmcabrita added a commit to amplemarket/resolv that referenced this pull request Oct 30, 2023
@rfgil
Copy link

rfgil commented Nov 24, 2023

This PR is useful, but we found a side effect that comes with it.

The initialiser skips the default resolv configuration when a hash is passed to it:

# Hash:: Must contain :nameserver, :search and :ndots keys.

This means consumers must also pass :nameserver, :search and :ndots when they want to set: :raise_timeout_erros.

We were not expecting this side effect and ended up removing the need for raise_timeout_erros. Now we're simply not rescuing theResolvTimeout error by default, letting the client handle it.

I can open a PR with the changes I just described if you find it useful.

Thanks!

@jeremyevans
Copy link
Contributor Author

How about the following diff?, which allows you to set the :raise_timeout_errors (and other config options) when calling Resolv.new, so you don't need to set the other options in order to use it:

diff --git a/lib/resolv.rb b/lib/resolv.rb
index ecf920b..84c3591 100644
--- a/lib/resolv.rb
+++ b/lib/resolv.rb
@@ -83,9 +83,18 @@ class Resolv
 
   ##
   # Creates a new Resolv using +resolvers+.
-
-  def initialize(resolvers=nil, use_ipv6: nil)
-    @resolvers = resolvers || [Hosts.new, DNS.new(DNS::Config.default_config_hash.merge(use_ipv6: use_ipv6))]
+  #
+  # If +resolvers+ is not given, a hash, or +nil+, uses a Hosts resolver and
+  # and a DNS resolver.  If +resolvers+ is a hash, uses the hash as
+  # configuration for the DNS resolver.
+
+  def initialize(resolvers=nil)
+    @resolvers = case resolvers
+    when Hash, nil
+      [Hosts.new, DNS.new(DNS::Config.default_config_hash.merge(resolvers || {}))]
+    else
+      resolvers
+    end
   end
 
   ##

The :use_ipv6 keyword was just added and not yet part of a release, so it should be fine to remove the keyword. The keyword still works the same way, but with the above approach, we avoid issues with specifying both the resolvers and the keyword (with the separate keyword, the keyword would be ignored if the resolvers option was given and not nil).

@jeremyevans
Copy link
Contributor Author

@rfgil I submitted a pull request for the above patch: #43

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

Successfully merging this pull request may close these issues.

4 participants