[PR #1823] [CLOSED] Possibility to have AsyncClient return SerialMessages #2639

New issue

Closed

opened 2026-03-16 11:00:02 +03:00 by kerem · 0 comments

kerem commented

2026-03-16 11:00:02 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/hickory-dns/hickory-dns/pull/1823
Author: @mattias-p
Created: 11/4/2022
Status: ❌ Closed

Base: main ← Head: generic-message

📝 Commits (10+)

cee5058 Extract message id from SerialMessage
720ffeb Generic struct DnsResponse
0afc69f Generic type TimeoutFuture
d67e290 Generic enum DnsResponseStreamInner
9e99d30 Generic struct DnsResponseStream
22fc5d7 Generic type MessageVerifier
480d21d Generic trait MessageFinalizer
27a8aab Generic trait DnsHandle
660ac5a Generic trait DnsRequestSender
8be939d Generic struct OneshotDnsResponse

📊 Changes

19 files changed (+462 additions, -260 deletions)

View changed files

📝 bin/tests/named_tests.rs (+4 -3)
📝 crates/client/src/client/async_client.rs (+108 -34)
📝 crates/client/src/client/client.rs (+25 -13)
📝 crates/client/src/client/client_connection.rs (+8 -3)
📝 crates/client/src/lib.rs (+3 -2)
📝 crates/client/src/rr/dnssec/signer.rs (+6 -4)
📝 crates/client/src/rr/dnssec/tsig.rs (+11 -5)
📝 crates/client/src/udp/udp_client_connection.rs (+15 -5)
📝 crates/proto/src/error.rs (+7 -0)
📝 crates/proto/src/op/message.rs (+8 -8)
📝 crates/proto/src/udp/udp_client_stream.rs (+67 -52)
📝 crates/proto/src/xfer/dns_exchange.rs (+59 -47)
📝 crates/proto/src/xfer/dns_handle.rs (+5 -2)
📝 crates/proto/src/xfer/dns_multiplexer.rs (+48 -34)
📝 crates/proto/src/xfer/dns_response.rs (+45 -25)
📝 crates/proto/src/xfer/mod.rs (+28 -19)
📝 crates/proto/src/xfer/serial_message.rs (+9 -0)
📝 crates/server/tests/authority_battery/dynamic_update.rs (+3 -1)
📝 tests/integration-tests/tests/client_future_tests.rs (+3 -3)

📄 Description

Here's a PoC for allowing AsyncClient to return responses as either Message or Vec.

The idea is to make DnsResponse and AsyncClient and everything in between generic over the response message representation so that AsyncClient can return either Messages or Vec. This PR is mostly just a big stack of commits adding a type parameter to one thing after another building up from DnsResponse to AsyncClient and adding little tweaks here and there to make things work. To start things off I made it so the response message id is extracted directly from the buffer (SerialMessage) instead of from the parsed Message - it's easy enough and we always have the buffer available in send_serial_message().

To avoid breaking lots of code I added Message as the default argument for the parameter. I don't know why this works in other places but doesn't work for AsyncClient.

I added a unit test as a quick and dirty example of usage. It probably has lots of room for improvement and it ought to be complemented with proper documentation.

When constructing an AsyncClient you have to choose between Message and Vec. Another option would be to let the caller decide for each call. This feel to me a bit like an artificial restriction, but I'm not sure what the use case would like for mixing calls for Messages and Vecs to the same AsyncClient.

I'm using impl TryFrom<Vec<u8>> for DnsResponse<M> parsing but we need to keep the Vec around so I clone it. I've focused on getting things to work first, deferring efficiency aspects at least for now.

I consistently added the type parameter to all the types themselves. In some cases it would probably better to add them to individual associated functions. For some traits maybe it's better to have them as associated types. Now that I have working code it's probably a good idea to revisit this aspect of the design.

Another thing I thought of but haven't looked into is making some deeper functions return Vec instead of DnsResponse. The potential benefit of this would be simplifying some of the type signatures. But maybe it's a terrible idea for other reasons.

I've only made things work for the AsyncClient. I haven't looked into the others.

Edit: Originally I implemented DnsResponse but I changed it to DnsResponse<Vec>. I also updated some of my reasoning in a few places.

Fixes #1814.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/hickory-dns/hickory-dns/pull/1823 **Author:** [@mattias-p](https://github.com/mattias-p) **Created:** 11/4/2022 **Status:** ❌ Closed **Base:** `main` ← **Head:** `generic-message` --- ### 📝 Commits (10+) - [`cee5058`](https://github.com/hickory-dns/hickory-dns/commit/cee5058a82197116846869655092223b1e35c3f9) Extract message id from SerialMessage - [`720ffeb`](https://github.com/hickory-dns/hickory-dns/commit/720ffebe9dd59cb7d929d4c238bbd9e7ac09dc0e) Generic struct DnsResponse - [`0afc69f`](https://github.com/hickory-dns/hickory-dns/commit/0afc69fccc341faf8a2485a01300f4e95d26e69f) Generic type TimeoutFuture - [`d67e290`](https://github.com/hickory-dns/hickory-dns/commit/d67e29056c585975a2697428e607bd07064c252c) Generic enum DnsResponseStreamInner - [`9e99d30`](https://github.com/hickory-dns/hickory-dns/commit/9e99d30312826748d20dfa22775ecdf8e922cc80) Generic struct DnsResponseStream - [`22fc5d7`](https://github.com/hickory-dns/hickory-dns/commit/22fc5d7d2d9b309b447a6c615fe6db992285b2d5) Generic type MessageVerifier - [`480d21d`](https://github.com/hickory-dns/hickory-dns/commit/480d21da0d27b94413c70b47e53bb1cf43b382ae) Generic trait MessageFinalizer - [`27a8aab`](https://github.com/hickory-dns/hickory-dns/commit/27a8aab19a5dd7d4c39cc3f327ee620b276300f9) Generic trait DnsHandle - [`660ac5a`](https://github.com/hickory-dns/hickory-dns/commit/660ac5a8f4b379f9232ec4374743f2a7cb0d9eeb) Generic trait DnsRequestSender - [`8be939d`](https://github.com/hickory-dns/hickory-dns/commit/8be939da2c6b92123bfeebbba79cd602bc5ae0f1) Generic struct OneshotDnsResponse ### 📊 Changes **19 files changed** (+462 additions, -260 deletions) <details> <summary>View changed files</summary> 📝 `bin/tests/named_tests.rs` (+4 -3) 📝 `crates/client/src/client/async_client.rs` (+108 -34) 📝 `crates/client/src/client/client.rs` (+25 -13) 📝 `crates/client/src/client/client_connection.rs` (+8 -3) 📝 `crates/client/src/lib.rs` (+3 -2) 📝 `crates/client/src/rr/dnssec/signer.rs` (+6 -4) 📝 `crates/client/src/rr/dnssec/tsig.rs` (+11 -5) 📝 `crates/client/src/udp/udp_client_connection.rs` (+15 -5) 📝 `crates/proto/src/error.rs` (+7 -0) 📝 `crates/proto/src/op/message.rs` (+8 -8) 📝 `crates/proto/src/udp/udp_client_stream.rs` (+67 -52) 📝 `crates/proto/src/xfer/dns_exchange.rs` (+59 -47) 📝 `crates/proto/src/xfer/dns_handle.rs` (+5 -2) 📝 `crates/proto/src/xfer/dns_multiplexer.rs` (+48 -34) 📝 `crates/proto/src/xfer/dns_response.rs` (+45 -25) 📝 `crates/proto/src/xfer/mod.rs` (+28 -19) 📝 `crates/proto/src/xfer/serial_message.rs` (+9 -0) 📝 `crates/server/tests/authority_battery/dynamic_update.rs` (+3 -1) 📝 `tests/integration-tests/tests/client_future_tests.rs` (+3 -3) </details> ### 📄 Description Here's a PoC for allowing AsyncClient to return responses as either Message or Vec<u8>. The idea is to make DnsResponse and AsyncClient and everything in between generic over the response message representation so that AsyncClient can return either Messages or Vec<u8>. This PR is mostly just a big stack of commits adding a type parameter to one thing after another building up from DnsResponse to AsyncClient and adding little tweaks here and there to make things work. To start things off I made it so the response message id is extracted directly from the buffer (SerialMessage) instead of from the parsed Message - it's easy enough and we always have the buffer available in send_serial_message(). To avoid breaking lots of code I added Message as the default argument for the parameter. I don't know why this works in other places but doesn't work for AsyncClient. I added a unit test as a quick and dirty example of usage. It probably has lots of room for improvement and it ought to be complemented with proper documentation. When constructing an AsyncClient you have to choose between Message and Vec<u8>. Another option would be to let the caller decide for each call. This feel to me a bit like an artificial restriction, but I'm not sure what the use case would like for mixing calls for Messages and Vecs to the same AsyncClient. I'm using `impl TryFrom<Vec<u8>> for DnsResponse<M>` parsing but we need to keep the Vec<u8> around so I clone it. I've focused on getting things to work first, deferring efficiency aspects at least for now. I consistently added the type parameter to all the types themselves. In some cases it would probably better to add them to individual associated functions. For some traits maybe it's better to have them as associated types. Now that I have working code it's probably a good idea to revisit this aspect of the design. Another thing I thought of but haven't looked into is making some deeper functions return Vec<u8> instead of DnsResponse<M>. The potential benefit of this would be simplifying some of the type signatures. But maybe it's a terrible idea for other reasons. I've only made things work for the AsyncClient. I haven't looked into the others. **Edit:** Originally I implemented DnsResponse<SerialMessage> but I changed it to DnsResponse<Vec<u8>>. I also updated some of my reasoning in a few places. Fixes #1814. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>