DefaultResolver gets ignored with excon 0.103.0

[robinmoneybird]

From what I can tell, is that after the changes made in #830, which are included in release 0.103.0, the Resolv DefaultResolver is ignored.

This change makes it impossible to use an overwritten default Resolv config (for example, in my use-case with a Rails initializer):

Resolv::DefaultResolver.replace_resolvers(
  [
    Resolv::Hosts.new,
    Resolv::DNS.new,
    Resolv::DNS.new(nameserver: ['127.0.0.1'])
  ]
)

Would it be possible to use the Resolv::DefaultResolver and add the DNS timeouts config to the predefined Resolv::DNS instances, or something that would make using a custom Resolv configuration possible?

[geemus]

Thanks for the detailed report, that is definitely an unintended consequence of our recent change.

I guess my inclination might be to at least partially revert the recent timeout related interface changes, and document for folks that they can instead use replace_resolvers as you have (with a clear example of setting the timeout on the DNS portions). I think that would work for everyone, but unfortunately is probably a breaking change from what we've just done (we could potentially do some deprecation-warning workaround before removing it I suspect). It's a bit more cumbersome interface-wise than the simple option and the backwards incompatibility is unfortunate, but it also seems much more flexible and futureproof.

What do you think?

[robinmoneybird]

Since this is something that is already possible to configure yourself outside this gem, I would find it logical to use that existing implementation, so that current setups which are already using default resolvers don't break in unexpected ways.

A notice in the documentation on how to configure this yourself with the default resolvers, with resolver.timeout=, would probably be the best option.

So while a revert is not ideal in terms of backwards compatibility, due to version 0.103.0, I think it outweighs default resolvers not working as expected.

[geemus]

@robinmoneybird Yeah, I think that makes sense.

@bruno-b-martins I also wanted to circle you in to get your thoughts on this since you drove the change. I will need to think a bit more on specifically how we would want to do this in terms of backwards compatibility and potential deprecation, but it seems like it might be the right thing to do. What do you think?

[bruno-b-martins]

Hey there,

I agree with @robinmoneybird that a revert outweighs the incompatibility issue introduced in v.0.103.0.

Since this change was introduced less than 1 week ago, it might be that nearly no one is using it yet.
I added it to avro_turf last week as well, but probably most people simply ignored it by now.

Thinking on @robinmoneybird's suggestion of setting the resolvers outside of excon, I get mixed feelings.
I see how it would work, but we would be transpiring the configuration of a dependency of excon to whatever application uses excon.
And, in my use case, I would have an initializer in my application to set up a gem (Resolv) that is used by a gem (Excon) that is used by a gem (AvroTurf). That configuration would also apply to the whole application, not only to Excon.
Meaning, if another gem would also use Resolv then, for the good and for the bad, it would be affected.

Probably there is a better solution for this. Something in the line of having Excon receive the Resolvers to use already set up (timeouts). And in Excon we would Resolv::DefaultResolver.replace_resolvers if these Resolvers are received.

[geemus]

@bruno-b-martins Yeah that makes sense.

It sounds like a possible in-between solution as suggested by @bruno-b-martins could be that we revert the dns_timeout argument changes, but instead add a new argument that expects an array of pre-configured resolvers.

This would allow the flexibility of making the sort of changes both of you are after, without having to change it for everything that might use it in your application. By default we could configure excon then to still use Resolv::DefaultResolver or instead use the specified resolvers. This would allow @robinmoneybird usage where the default resolvers are set and it is expected to apply to excon and @bruno-b-martins usage where they would like to more directly and narrowly set things.

@bruno-b-martins @robinmoneybird does that sound like a good approach?

[bruno-b-martins]

To me it sounds like a good approach. Do you want help?
I can probably dedicate some time in the beginning of next week.

[geemus]

@bruno-b-martins I'd love help if you have some capacity. I have a 3 week old at home and am heading to a tech conference for the later half of this week, so I don't think I'll have a chance to do much with it until at least mid next week otherwise.

[bruno-b-martins]

Wow! Congrats 😊
Focus on the important stuff for now.

I can probably start the PR next week and I'll ping you when in doubt. Reviewing should take less time than developing and testing 😅

[geemus]

@bruno-b-martins thanks. Sounds great, just let me know if you have questions (as you suggest, that's much easier to find time and brain power for).

[geemus]

I believe this is now addressed by #834 and released in v0.104.0.

[robinmoneybird]

@geemus, @bruno-b-martins, thank you for the quick turn-around on this issue.