starred/hickory-dns

Fork 0

mirror of https://github.com/hickory-dns/hickory-dns.git synced 2026-04-25 03:05:51 +03:00

[GH-ISSUE #1933] Question: Mutex around AsyncClient #823

New issue

Closed

opened 2026-03-16 00:25:59 +03:00 by kerem · 7 comments

kerem commented

2026-03-16 00:25:59 +03:00

Owner

Originally created by @andrewbaxter on GitHub (May 15, 2023).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/1933

AsyncClient::query takes a mutable self. Since AsyncClient will probably be used across threads, I'd expect it to be immutable. AFAICT wrapping in Mutex works fine, but it means you can't do client.lock().unwrap().query().await since the guard stays around until the query completes (you need to store client.lock().unwrap().query() in one statement then await it in the next).

Should I be doing something else here?

For background, I'm writing the classic "DNS server that handles some requests locally and forwards the rest to 1.1.1.1".

Originally created by @andrewbaxter on GitHub (May 15, 2023). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/1933 `AsyncClient::query` takes a mutable self. Since `AsyncClient` will probably be used across threads, I'd expect it to be immutable. AFAICT wrapping in `Mutex` works fine, but it means you can't do `client.lock().unwrap().query().await` since the guard stays around until the query completes (you need to store `client.lock().unwrap().query()` in one statement then await it in the next). Should I be doing something else here? For background, I'm writing the classic "DNS server that handles some requests locally and forwards the rest to 1.1.1.1".

kerem

2026-03-16 00:25:59 +03:00

closed this issue
added the
question
label

kerem commented

2026-03-16 00:26:20 +03:00

Author

Owner

@djc commented on GitHub (May 15, 2023):

You probably want to use a resolver rather than a client? AsyncResolver::lookup() takes &self.

@djc commented on GitHub (May 15, 2023): You probably want to use a resolver rather than a client? [`AsyncResolver::lookup()`](https://docs.rs/trust-dns-resolver/latest/trust_dns_resolver/struct.AsyncResolver.html#method.lookup) takes `&self`.

kerem commented

2026-03-16 00:26:25 +03:00

Author

Owner

@andrewbaxter commented on GitHub (May 15, 2023):

It seemed like it would be easier to map the parameters and return values from the values the AsyncClient uses to that of the RequestHandler if I'm just forwarding. The simplified interface doesn't take a lot of the request information and doesn't include a lot of the response information (authority, additional records, etc).

I'm doing

FutureServer -> MyHandler -> AsyncClient

maybe that architecture is mistaken.

@andrewbaxter commented on GitHub (May 15, 2023): It seemed like it would be easier to map the parameters and return values from the values the `AsyncClient` uses to that of the `RequestHandler` if I'm just forwarding. The simplified interface doesn't take a lot of the request information and doesn't include a lot of the response information (authority, additional records, etc). I'm doing ``` FutureServer -> MyHandler -> AsyncClient ``` maybe that architecture is mistaken.

kerem commented

2026-03-16 00:26:30 +03:00

Author

Owner

@djc commented on GitHub (May 15, 2023):

Fair point. I think it would probably make sense that AsyncClient::query() should take &self and use some inner mechanism to deal with mutability (it looks like it just holds an mpsc::Sender under the covers, which could just be cloned). If you want to work through what that needs, we'll definitely review your PR. I think the -client crate is relatively rarely used so there might still be reasons to look into -resolver instead depending on your exact use cases.

@djc commented on GitHub (May 15, 2023): Fair point. I think it would probably make sense that `AsyncClient::query()` should take `&self` and use some inner mechanism to deal with mutability (it looks like it just holds an `mpsc::Sender` under the covers, which could just be cloned). If you want to work through what that needs, we'll definitely review your PR. I think the -client crate is relatively rarely used so there might still be reasons to look into -resolver instead depending on your exact use cases.

kerem commented

2026-03-16 00:26:35 +03:00

Author

Owner

@andrewbaxter commented on GitHub (May 15, 2023):

Okay cool, yeah. I haven't gotten very far but I'll come back to this once I've gotten my own code more stable.

Would it make more sense to provide an additional alternative query method in resolver that takes the same arguments as Client does for it's query (like full_lookup or raw_lookup or advanced_lookup or something)?

I'm wondering if I shouldn't use resolver instead anyways for the things like using multiple servers, etc. I'm not sure if resolver has a simple interface just for user friendliness or out of necessity due to abstraction over multiple disparate backends.

@andrewbaxter commented on GitHub (May 15, 2023): Okay cool, yeah. I haven't gotten very far but I'll come back to this once I've gotten my own code more stable. Would it make more sense to provide an additional alternative query method in resolver that takes the same arguments as Client does for it's query (like `full_lookup` or `raw_lookup` or `advanced_lookup` or something)? I'm wondering if I shouldn't use resolver instead anyways for the things like using multiple servers, etc. I'm not sure if resolver has a simple interface just for user friendliness or out of necessity due to abstraction over multiple disparate backends.

kerem commented

2026-03-16 00:26:40 +03:00

Author

Owner

@djc commented on GitHub (May 15, 2023):

Probably more for the simple interface, but you'll probably only find out for sure if you try! I think some kind of full_lookup() (naming to be bikeshedded) probably makes sense.

@djc commented on GitHub (May 15, 2023): Probably more for the simple interface, but you'll probably only find out for sure if you try! I think some kind of `full_lookup()` (naming to be bikeshedded) probably makes sense.

kerem commented

2026-03-16 00:26:45 +03:00

Author

Owner

@ibigbug commented on GitHub (Sep 4, 2023):

I'm also facing the issue, made a PR here https://github.com/bluejekyll/trust-dns/pull/2018

@ibigbug commented on GitHub (Sep 4, 2023): I'm also facing the issue, made a PR here https://github.com/bluejekyll/trust-dns/pull/2018

kerem commented

2026-03-16 00:26:50 +03:00

Author

Owner

@andrewbaxter commented on GitHub (Sep 23, 2023):

I think this is done in #2018 - I haven't checked but I'll close for now.

@andrewbaxter commented on GitHub (Sep 23, 2023): I think this is done in #2018 - I haven't checked but I'll close for now.

kerem referenced this issue

2026-03-16 02:21:24 +03:00

[PR #823] [MERGED] Bump tokio from 0.1.21 to 0.1.22 #1713