[GH-ISSUE #227] Synchronous resolver is not Send + Sync

kerem commented

2026-03-15 22:19:01 +03:00

Owner

Originally created by @mathstuf on GitHub (Oct 12, 2017).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/227

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.

Originally created by @mathstuf on GitHub (Oct 12, 2017). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/227 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.

kerem

2026-03-15 22:19:01 +03:00

closed this issue
added the
perf

bug

crate:client

crate:proto
labels

kerem commented

2026-03-15 22:19:17 +03:00

Author

Owner

@mathstuf commented on GitHub (Oct 12, 2017):

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

@mathstuf commented on GitHub (Oct 12, 2017): Yeah…even the base `trust-dns` synchronous client isn't `Send + Sync`.

kerem commented

2026-03-15 22:19:22 +03:00

Author

Owner

@bluejekyll commented on GitHub (Oct 12, 2017):

It is cloneable. Does clone not facilitate your needs?

@bluejekyll commented on GitHub (Oct 12, 2017): It is cloneable. Does clone not facilitate your needs?

kerem commented

2026-03-15 22:19:27 +03:00

Author

Owner

@mathstuf commented on GitHub (Oct 12, 2017):

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 commented on GitHub (Oct 12, 2017): 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.

kerem commented

2026-03-15 22:19:32 +03:00

Author

Owner

@mathstuf commented on GitHub (Oct 12, 2017):

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

@mathstuf commented on GitHub (Oct 12, 2017): FWIW, we require `Sync` here for other reasons, but it's not something that's easily fixable without making everything single threaded again.

kerem commented

2026-03-15 22:19:37 +03:00

Author

Owner

@bluejekyll commented on GitHub (Oct 12, 2017):

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.

@bluejekyll commented on GitHub (Oct 12, 2017): 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 *`Handle`s 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.

kerem commented

2026-03-15 22:19:42 +03:00

Author

Owner

@mathstuf commented on GitHub (Oct 12, 2017):

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 commented on GitHub (Oct 12, 2017): 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.

kerem commented

2026-03-15 22:19:48 +03:00

Author

Owner

@mathstuf commented on GitHub (Oct 12, 2017):

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

@mathstuf commented on GitHub (Oct 12, 2017): I've filed tokio-rs/tokio-core#265 to help improve Tokio's documentation about this problem.

kerem commented

2026-03-15 22:19:53 +03:00

Author

Owner

@bluejekyll commented on GitHub (Oct 12, 2017):

We might be able to "fix" the SyncClient to use a thread-local Core, https://github.com/tokio-rs/tokio-core/pull/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?

@bluejekyll commented on GitHub (Oct 12, 2017): We might be able to "fix" the SyncClient to use a thread-local Core, https://github.com/tokio-rs/tokio-core/pull/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?

kerem commented

2026-03-15 22:19:58 +03:00

Author

Owner

@mathstuf commented on GitHub (Oct 12, 2017):

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.

@mathstuf commented on GitHub (Oct 12, 2017): 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 `Core`s all the time is an issue.

kerem commented

2026-03-15 22:20:03 +03:00

Author

Owner

@bluejekyll commented on GitHub (Oct 12, 2017):

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.

@bluejekyll commented on GitHub (Oct 12, 2017): > 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.

kerem commented

2026-03-15 22:20:08 +03:00

Author

Owner

@mathstuf commented on GitHub (Oct 13, 2017):

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.

@mathstuf commented on GitHub (Oct 13, 2017): > 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`.

kerem commented

2026-03-15 22:20:13 +03:00

Author

Owner

@bluejekyll commented on GitHub (Oct 13, 2017):

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.

@bluejekyll commented on GitHub (Oct 13, 2017): I think it's reasonable to put thread-local `Core`s into the synchronous variants of the library. I'll open a separate issue for that.

kerem commented

2026-03-15 22:20:18 +03:00

Author

Owner

@mathstuf commented on GitHub (Oct 13, 2017):

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

@mathstuf commented on GitHub (Oct 13, 2017): I'd really rather not have a library setting up threads underneath me.

kerem commented

2026-03-15 22:20:23 +03:00

Author

Owner

@mathstuf commented on GitHub (Oct 13, 2017):

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

@mathstuf commented on GitHub (Oct 13, 2017): (Without it being something the library is meant to do, like `rayon`.)

kerem commented

2026-03-15 22:20:28 +03:00

Author

Owner

@bluejekyll commented on GitHub (Oct 13, 2017):

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.

@bluejekyll commented on GitHub (Oct 13, 2017): 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.

kerem commented

2026-03-15 22:20:33 +03:00

Author

Owner

@briansmith commented on GitHub (Oct 24, 2017):

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]

@briansmith commented on GitHub (Oct 24, 2017): 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`]

kerem commented

2026-03-15 22:20:38 +03:00

Author

Owner

@bluejekyll commented on GitHub (Oct 24, 2017):

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

@bluejekyll commented on GitHub (Oct 24, 2017): I think I should be able to achieve both with the current implementation...

kerem commented

2026-03-15 22:20:43 +03:00

Author

Owner

@mathstuf commented on GitHub (Oct 24, 2017):

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

@mathstuf commented on GitHub (Oct 24, 2017): Using `Rc` to make a `Sync` thing into `Send` where sharing isn't necessary is, IMNSHO, a hack and shouldn't be necessary.

kerem commented

2026-03-15 22:20:49 +03:00

Author

Owner

@briansmith commented on GitHub (Oct 24, 2017):

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.)

@briansmith commented on GitHub (Oct 24, 2017): > 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.)

kerem commented

2026-03-15 22:20:54 +03:00

Author

Owner

@bluejekyll commented on GitHub (Oct 24, 2017):

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.

@bluejekyll commented on GitHub (Oct 24, 2017): 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`.

kerem commented

2026-03-15 22:20:59 +03:00

Author

Owner

@briansmith commented on GitHub (Oct 24, 2017):

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 commented on GitHub (Oct 24, 2017): > 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.

kerem commented

2026-03-15 22:21:04 +03:00

Author

Owner

@briansmith commented on GitHub (Oct 24, 2017):

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.

@briansmith commented on GitHub (Oct 24, 2017): 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.

kerem commented

2026-03-15 22:21:09 +03:00

Author

Owner

@bluejekyll commented on GitHub (Oct 24, 2017):

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 commented on GitHub (Oct 24, 2017): 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.

kerem commented

2026-03-15 22:21:14 +03:00

Author

Owner

@bluejekyll commented on GitHub (Oct 27, 2017):

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 commented on GitHub (Oct 27, 2017): 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.

kerem commented

2026-03-15 22:21:19 +03:00

Author

Owner

@bluejekyll commented on GitHub (Oct 31, 2017):

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.

@bluejekyll commented on GitHub (Oct 31, 2017): 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.

kerem referenced this issue

2026-03-16 02:05:08 +03:00

[PR #402] [MERGED] instead of infinite notify loop, use timeout in mock #1415

Rows
Columns

[GH-ISSUE #227] Synchronous resolver is not Send + Sync #402