Synchronous resolver is not Send + Sync

[mathstuf]

This is due to its embedding of the async Reactor from Tokio. This means that for my Send + Sync structure, I need to instead create a resolver on each call instead of being able to amortize it to be constructed just once.

[mathstuf]

Yeah…even the base trust-dns synchronous client isn't Send + Sync.

[bluejekyll]

It is cloneable. Does clone not facilitate your needs?

[mathstuf]

No, Clone doesn't suffice. I have a setup which runs things via rayon. One of these things happens to do DNS lookups (using dig via fork/exec right now). The structure that does the lookups cannot store a Client in it because rayon requires Send at least which is not possible with tokio's Core structure.

[mathstuf]

FWIW, we require Sync here for other reasons, but it's not something that's easily fixable without making everything single threaded again.

[bluejekyll]

So you may be bumping into some areas where I haven't had any strong use cases, and so don't have any good examples at the moment. We don't have any yet, but perhaps you wouldn't mind creating an example that we could put into resolver/examples (doesn't exist yet, but it would be cool to start a set of examples there).

To your specific issue. You may need to use ResolverFuture directly. I have an example here: https://docs.rs/trust-dns-resolver/0.6.0/trust_dns_resolver/#using-the-tokioasync-resolver . As of now, the ResolverFuture isn't multi-Core aware. It spawns itself and returns a *Handle (basically a Sink in proper Tokio parlance) which can be used to send messages to the spawned instance of the Future. The ResolverFuture should shutdown once all outstanding *Handles are dropped. I think in your case, you'd want to initialize a ResolverFuture on one Core then, clone each Handle into the new threads. To run, you'd end up with a Core per thread as well.

I haven't worked much with rayon yet. But it would be really awesome to prove out the best method for working with it! It's also very possible you're going to bump into some limitations we're going to have to fix.

[mathstuf]

Basically, the synchronous and asynchronous implementations can't share "business logic" code. They can share parsers and other pure logic, but the internal Core required to use the async code via a sync interface blocks concurrency via thread workers due to the sharing restrictions. And async can't use sync business logic due to the blocking nature of it.

[mathstuf]

I've filed tokio-rs/tokio-core#265 to help improve Tokio's documentation about this problem.

[bluejekyll]

We might be able to "fix" the SyncClient to use a thread-local Core, tokio-rs/tokio-core#212 , or something similar. If we remove Core from the SyncClient, then we might have some options here. This is meant to exist as an ease of use option so that people don't necessarily have to get involved with Tokio, obviously making it more flexible for cases like threading, etc. would be nice, but I also see that as slightly more advanced, and therefor pushing towards Tokio at that point doesn't seem inappropriate to me.

btw, is my above post about using ResolverFuture an option for you?

[mathstuf]

I'm not thrilled about those changes to tokio…Rust the language doesn't have a runtime that needs initialized (IIRC, the main reason green threads were dropped pre-1.0) and I don't think a lazily initialized one is good either. Crates certainly shouldn't be relying on such global, implicit behavior IMO.

btw, is my above post about using ResolverFuture an option for you?

Possibly, I'll need to test it. I also might not be able to practically use it if creating and destroying Cores all the time is an issue.

[bluejekyll]

if creating and destroying Cores all the time is an issue

You could potentially (I haven't done this) just stick the Core in a thread local, then you'd only ever create one per thread.

[mathstuf]

You could potentially (I haven't done this) just stick the Core in a thread local, then you'd only ever create one per thread.

Eh, the extra complexity there probably makes it easier to just stick with dig behind fork and exec.

[bluejekyll]

I think it's reasonable to put thread-local Cores into the synchronous variants of the library. I'll open a separate issue for that.

[mathstuf]

I'd really rather not have a library setting up threads underneath me.

[mathstuf]

(Without it being something the library is meant to do, like rayon.)

[bluejekyll]

Thread locals don’t set up a new thread, they associate static context to the current thread. This would allow a single Core per thread to exist, without any work by the user of the lib.

No new thread creation at all.

edit: the #245 doesn't use ThreadLocal, but does create a new Core on each Sync request.

[briansmith]

Do you really need the sync resolver to be Send? If it is made Sync, then you can use Arc<...> to reference the resolver in your Send + Sync structure. IMO, that's quite a reasonable trade-off. If you can't use Arc<...> to reference the sync resolver then it would be good to know why.

[Edit: s/Rc/Arc]

[bluejekyll]

I think I should be able to achieve both with the current implementation...

[mathstuf]

Using Rc to make a Sync thing into Send where sharing isn't necessary is, IMNSHO, a hack and shouldn't be necessary.

[briansmith]

Using Rc to make a Sync thing into Send where sharing isn't necessary is, IMNSHO, a hack and shouldn't be necessary.

When making a complex type Send internally, we tend to end up either needing to either clone internal stuff or wrap it in Arc or Rc. So actually, asking the few users who need the Send functionality to implement it themselves in terms of Arc ends up making the internals simpler and more efficient for the users that don't need it. (In this case, I think it's mostly a non-issue since we can just wrap a couple of internal things in Arc and be done with it.)

[bluejekyll]

Though, the point is good. I'd love to be able to remove Arc where possible to reduce the overhead on the pointer type in cases where possible. So if we did only support Sync that might be more efficient, and thus allow the async variants to not incur the cost of the Arc.

[briansmith]

Though, the point is good. I'd love to be able to remove Arc where possible to reduce the overhead on the pointer type in cases where possible. So if we did only support Sync that might be more efficient, and thus allow the async variants to not incur the cost of the Arc.

First, I mixed up Send and Sync. The extra cloning in the current PR for this is to make things Sync, not to make things Send.

Second, I think I was confused about whether Arc helps us get Sync or Send for free. Mutex actually gives us Sync for almost free for any time that is already Send. That has nothing to do with Arc. Sorry.

I think it shouldn't cost much (nothing?) to make the async types Send and that's probably necessary for good Tokio support.

It is reasonable to either ask the users of the synchronous API to use Mutex when they need Sync, or it would be reasonable to implement the synchronous API in a way that uses a Mutex around the async API for those users, but I don't think it's reasonable to add Arc or do extra cloning/copying in the async code just to support a Sync synchronous API. Using a Mutex would be cheaper anyway.

[briansmith]

Also, if we make the async API Sync now, then we'll have to add Mutex internally later if/when we support things like PKCS#11 or other things that aren't as nicely thread-safe as Rust. That seems like a high long-term cost, even if we can avoid it straightforwardly now.

[bluejekyll]

Let me fix the current issues in the PR, which is just to make it Send at the moment, if it ends up looking like it's too much cost internally, then we can decide what other options we have.

[bluejekyll]

I've run into some issues with OpenSSL in the current PR. It will most likely effect NativeTLS as well.

There are two options moving forward, only support Rustls for client TLS, or requrie Arc<Mutex<Resolver>>.

I'm also not a fan of the implementation in that PR at the moment, especially for TCP, which will require re-connections for each DNS request. I might try a different direction tomorrow.

[bluejekyll]

Ok, #245 is the path forward on this.

If anyone is interested in the changes, please look at that PR. It makes the Resolver and Client Send+Sync, without any static information or shared Core's. If you have feedback please file it soon, otherwise I'll merge this in, and it will be incorporated in the next release.

By the way, the only supported SyncClient TLS implementation will be Rustls after this change. For the others, the tokio variants will be required.