Client Transport #215
Client Transport #215
Conversation
Philip-NLnetLabs
force-pushed
the
transport
branch
from
38b11a7
to
a647108
Compare
Philip-NLnetLabs
force-pushed
the
transport
branch
from
02c8291
to
891c862
Compare
There was a problem hiding this comment.
This is only a partial review just yet due to time constraints – I have only looked at the udp and tcp_mutex modules (plus some supporting stuff) yet.
A general note about code organization. We currently keep type definitions and their impl blocks together – I think that makes it easier to find stuff and read code in order – and separate these into “chapters” split by dashed line comments (see other modules for examples).
src/net/client/factory.rs
Outdated
| /// This method is equivalent to async fn next(&self) -> Result<IO, std::io::Error>; | ||
| fn next( | ||
| &self, | ||
| ) -> Pin<Box<dyn Future<Output = Result<IO, std::io::Error>> + Send + '_>>; |
There was a problem hiding this comment.
Is there a good reason to always return a boxed trait object here rather than having the type of the future as a associated type (which can then still by a boxed trait object if the implementer prefers)?
Also, I’m not sure about the Pin there. Given that IO is Unpin, this type can probably also be Unpin?
There was a problem hiding this comment.
I replaced the Pin<Box< with an associated type. I don't know how to get rid of the Pin<Box< in the implementation. So it doesn't seem to have any net result.
There was a problem hiding this comment.
Async trait functions will be stabilized real soon (next release?), so maybe we should just switch to that, use the feature flag for now and only release one it is stable?
There was a problem hiding this comment.
It would be annoying to roll back if we need to release before async trait functions are released. And it should be a small amount of work when they are released. So maybe for now we can concentrate on other parts. The factory is implemented in only two places and I guess for using it, it doesn't really matter.
src/net/client/tcp_mutex.rs
Outdated
| // Index in the vector where to look for a space for a new query | ||
| curr: usize, | ||
|
|
||
| vec: Vec<Option<SingleQuery>>, |
There was a problem hiding this comment.
Looks a bit like you are making your own version of a Slab (which is used by Tokio, anyway, so you can use it without worrying about an extra dependency).
src/net/client/tcp_mutex.rs
Outdated
| } | ||
|
|
||
| struct InnerTcpConnection { | ||
| stream: Std_mutex<Option<TcpStream>>, |
There was a problem hiding this comment.
Shouldn’t you be using Tokio mutexes here? You seem to be calling lock() in async code further which may block a runtime thread. This is also the proper fix for all the await_holding_lock Clippy lint triggered below.
The Tokio mutex has a blocking_lock method for use outside of an async context.
src/net/client/tcp_mutex.rs
Outdated
| break; | ||
| } | ||
|
|
||
| let read_res = sock.read_buf(&mut buf).await; |
There was a problem hiding this comment.
I think this can end up reading more than len bytes if the sender side manages to make bigger frames. It is probably safer (in particular because you panic if this goes wrong) if you create a properly sized buffer and read_exact() into it, even if this means unnecessarily initializing memory for now.
src/net/client/tcp_mutex.rs
Outdated
| // Check if we are done at the head of the loop | ||
| } | ||
|
|
||
| let reply_message = Message::<Bytes>::from_octets(buf.into()); |
There was a problem hiding this comment.
No need to stick the message into a local variable here, you can just match on the function call.
Philip-NLnetLabs
force-pushed
the
transport
branch
from
6338031
to
d387b26
Compare
asynchronous functions in AsyncDgramRecvEx and AsyncDgramSendEx.
…cept one byte at a time.
Breaking changes * All types and functions referring to domain names have been changed from using the term “dname” to just “name.” For instance, `Dname` has become `Name`, `ToDname` has become `ToName`, and `ToDname::to_dname` has become `ToName::to_name`. ([#290]) * The `ToName` and `ToRelativeName` traits have been changed to have a pair of methods a la `try_to_name` and `to_name` for octets builders with limited and unlimited buffers, reflecting the pattern used elsewhere. ([#285]) * The types for IANA-registered parameters in `base::iana` have been changed from enums to a newtypes around their underlying integer type and associated constants for the registered values. (This was really always the better way to structure this.) ([#276], [#298]) * The `Txt` record data type now rejects empty record data as invalid. As a consequence `TxtBuilder` converts an empty builder into TXT record data consisting of one empty character string which requires `TxtBuilder::finish` to be able to return an error. ([#267]) * `Txt` record data serialization has been redesigned. It now serialized as a sequence of character strings. It also deserializes from such a sequence. If supported by the format, it alternatively deserializes from a string that is broken up into 255 octet chunks if necessary. ([#268]) * The text formatting for `CharStr` has been redesigned. The `Display` impl now uses a modified version of the representation format that doesn’t escape white space but also doesn’t enclose the string in quotes. Methods for explicitly formatting in quoted and unquoted presentation format are provided. ([#270]) * The `validate::RrsigExt` trait now accepts anything that impls `AsRef<Record<..>>` to allow the use of smart pointers. ([#288] by [@hunts]) * The stub resolver now uses the new client transports. This doesn’t change how it is used but does change how it queries the configured servers. ([#215]) * The sub resolver’s server configuration `Transport` type has been changed to be either `Transport::UdpTcp` for trying UDP and if that leads to a truncated answer try TCP and `Transport::Tcp` for only trying TCP. The stub resolver uses these accordingly now ([#296]) * Many error types have been changed from enums to structs that hide internal error details. Enums have been kept for errors where distinguishing variants might be meaningful for dealing with the error. ([#277]) * Renamed `Dnskey::is_zsk` to `is_zone_key`. ([#292]) * Split RRSIG timestamp handling from `Serial` into a new type `rdata::dnssec::Timestamp`. ([#294]) * Upgraded `octseq` to 0.5. ([#257]) * The minimum Rust version is now 1.70. ([#304]) New * Add impls for `AsRef<RelativeDname<[u8]>>` and `Borrow<RelativeDname<[u8]>>` to `RelativeDname<_>`. ([#251] by [@torin-carey]) * Added `name::Chain::fmt_with_dots` to format an absolute chained name with a final dot. ([#253]) * Added a new `ParseAnyRecordData` trait for record data types that can parse any type of record data. ([#256]) * Added implementations of `OctetsFrom` and `Debug` to `AllOptData` and the specific options types that didn’t have them yet. ([#257]) * Added missing ordering impls to `ZoneRecordData`, `AllRecordData`, `Opt`, and `SvcbRdata`. ([#293]) * Added `Name::reverse_from_addr` that creates a domain name for the reverse lookup of an IP address. ([#289]) * Added `OptBuilder::clone_from` to replace the OPT record with the content of another OPT record. ([#299]) * Added `Message::for_slice_ref` that returns a `Message<&[u8]>`. ([#300]) Bug fixes * Fixed the display implementation of `name::Chain<_, _>`. ([#253]) * Fixed the display implementation of `rdata::Txt<..>`. It now displays each embedded character string separately in quoted form. ([#259]) * Fixed the extended part returned by `OptRcode::to_parts` (it was shifted by 4 bits too many) and return all 12 bits for the `Int` variant in `OptRcode::to_int`. ([#258]) * Fixed a bug in the `inplace` zonefile parser that made it reject character string of length 255. ([#284]) Unstable features * Added the module `net::client` with experimental support for client message transport, i.e., sending of requests and receiving responses as well as caching of responses. This is gated by the `unstable-client-transport` feature. ([#215],[#275]) * Added the module `net::server` with experimental support for server transports, processing requests through a middleware chain and a service trait. This is gated by the `unstable-server-transport` feature. ([#274]) * Added the module `zonetree` providing basic traits representing a collection of zones and their data. The `zonetree::in_memory` module provides an in-memory implementation. The `zonetree::parsed` module provides a way to classify RRsets before inserting them into a tree. This is gated by the `unstable-zonetree` feature. ([#286])
No description provided.