Provider API Redesign

[XOR-op]

In the later discussion of #1876, @jeff-hiner argues that the current Provider API doesn't offer a convenient way to statefully create DnsHandle. This PR redesigns the Provider API.

This PR replaces the original CreateConnection trait with the new factory trait ConnectionProvider, allowing stateful connection creation. Now we have two layers of customization, ConnectionProvider and RuntimeProvider. The RuntimeProvider is a factory to create TCP or UDP connections, as well as runtime-specific timer. The ConnectionProvider is a factory to create a DnsHandle with the help of RuntimeProvider.

pub trait ConnectionProvider: 'static + Clone + Send + Sync + Unpin {
    /// The handle to the connect for sending DNS requests.
    type Conn: DnsHandle<Error = ResolveError> + Clone + Send + Sync + 'static;
    /// Ths future is responsible for spawning any background tasks as necessary.
    type FutureConn: Future<Output = Result<Self::Conn, ResolveError>> + Send + 'static;
    /// Provider that handles the underlying I/O and timing.
    type RuntimeProvider: RuntimeProvider;

    /// Create a new connection.
    fn new_connection(&self, config: &NameServerConfig, options: &ResolverOpts)
        -> Self::FutureConn;
}

If a user wants to override the default networking behavior, he/she can simply implement a new RuntimeProvider, without the need of re-implementing the GenericConnection internal logic. If a user wants to override the upper logic of a DnsHandle, for example add an auth field to the HTTP header, he/she can just implement a new ConnectionProvider and reuse current RuntimeProvider (It's also possible not to use a connection provided by RuntimeProvider, as long as the user insists).

Currently, the default handle is GenericConnection and the default connection provider is GenericProvider.

This PR also clarifies the usage of Provider API. The original comment in 0.22 as well as #1876 is kind of misleading.

[XOR-op]

@jeff-hiner I have created a new API based on your comments. Please check it out and see if there is still something wrong.

[bluejekyll]

@jeff-hiner, would you be able to provide feedback on this?

[jeff-hiner]

I'll take a look today.

[XOR-op]

@jeff-hiner Is there any feedback?

[jeff-hiner]

Sorry for the delay. This seems to work really well with very minimal changes required on my end. Thank you SO MUCH for your hard work here!

[bluejekyll]

This is great news. Thank you @jeff-hiner for taking the time to review. And @XOR-op, thank you for spending all the time to clean all of this up for this use case.

Do either of you think there might be a way for us to put in place a test or something that we could mark as "do not modify" as it's a public API that would be expensive to change, etc? I guess I'm asking, how can we avoid folks ending up spending time on similar things happing with API changes in the future?

[XOR-op]

Thanks Jeff for your feedback! And @bluejekyll I have rebased to make this PR up-to-date, so that you could give it a code review.

As for how to keep the functionality of public API stable, I think there are a few things we can do. First, I think is we need to check if some docs are misleading. The original doc of ConnectionProvider says it is "Needed mainly for mocking purposes." which we really should avoid. I think the only acceptable expressions are "should ONLY be used for internal use" if we have to expose as pub, or just say nothing.

Beyond that, test is always good for compatibility. If someone wants to change a API and we don't want to lose compatibility, there should be an existent test. Otherwise we should add extra tests before changing this API. Also we can distinguish user-level test from internal test. Internal tests are used to verify the internal correctness, while user-level tests verify if users could use public API as intended. I think user-level test can be put into examples/. But I'm not sure whether we can divide existing tests into these two categories clearly.

[jeff-hiner]

Yeah, this one is a bit rough because honestly I'm abusing the internals of the crate in order to do things it wasn't designed to do, when instead I originally should have PR'd in a fix to add a config field to have trust-dns inject the https headers.

Do we want to maybe mark these internals as #[deprecated] in the next release, with some loud rustdoc that indicates they may not be public in future releases?

[XOR-op]

I think for this case, it's still a valid need. Although originally it's for mocking purpose, I don't see this interface will prohibit any future design and I don't see we will gain any from the deprecation to my understanding. But if we need to deprecate it finally, we should implement appropriate alternatives for that, although it could be hard to know how users use this interface. My mistake is in previous PR I thought such case could also be implemented with some efforts without losing compatibility, but indeed it failed.

[bluejekyll]

I like the idea of adding an example for this. If either of you have time for that, I think it would be useful. And then we could note that the example should not significantly change the internal API given valid external use cases for it.

We can merge this as is, but if either of you have time to add that example, that would be awesome. Again, thank you both for your effort on this.

[bluejekyll]

Looks good. Mostly a bunch of renaming. It does simplify some of the generic interfaces which is a good improvement.

I'm good with this. @djc, any concerns?

[djc]

I don't have time to dig through this again, it sounds like the issues have been resolved though!