From 46c917d127519fbf917c391afc30fa83c794061e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 7 Dec 2021 14:03:14 -0500 Subject: [PATCH 1/2] Rename timeout_rules to stream_timeouts. (There are other timeout rules, after all.) Also, rename stream_timeout to connect_timeout, to make it more clear when it applies. --- crates/arti-client/src/client.rs | 4 +-- crates/arti-client/src/config.rs | 31 ++++++++++++----------- crates/arti-config/src/arti_defaults.toml | 10 +++++--- crates/arti-config/src/options.rs | 22 ++++++++-------- 4 files changed, 36 insertions(+), 31 deletions(-) diff --git a/crates/arti-client/src/client.rs b/crates/arti-client/src/client.rs index 6439e7b86..af1bdebdb 100644 --- a/crates/arti-client/src/client.rs +++ b/crates/arti-client/src/client.rs @@ -173,7 +173,7 @@ impl TorClient { ); } let addr_cfg = config.address_filter.clone(); - let timeout_cfg = config.timeout_rules.clone(); + let timeout_cfg = config.stream_timeouts.clone(); let chanmgr = Arc::new(tor_chanmgr::ChanMgr::new(runtime.clone())); let circmgr = tor_circmgr::CircMgr::new(circ_cfg, statemgr.clone(), &runtime, Arc::clone(&chanmgr))?; @@ -265,7 +265,7 @@ impl TorClient { // This timeout is needless but harmless for optimistic streams. let stream = self .runtime - .timeout(self.timeoutcfg.stream_timeout, stream_future) + .timeout(self.timeoutcfg.connect_timeout, stream_future) .await??; Ok(stream) diff --git a/crates/arti-client/src/config.rs b/crates/arti-client/src/config.rs index cc45f2532..35062ccbc 100644 --- a/crates/arti-client/src/config.rs +++ b/crates/arti-client/src/config.rs @@ -57,9 +57,9 @@ pub struct ClientAddrConfig { pub struct ClientTimeoutConfig { /// How long should we wait before timing out a stream when connecting /// to a host? - #[builder(default = "default_dns_stream_timeout()")] - #[serde(with = "humantime_serde", default = "default_dns_stream_timeout")] - pub stream_timeout: Duration, + #[builder(default = "default_connect_timeout()")] + #[serde(with = "humantime_serde", default = "default_connect_timeout")] + pub connect_timeout: Duration, /// How long should we wait before timing out when resolving a DNS record? #[builder(default = "default_dns_resolve_timeout()")] @@ -109,7 +109,7 @@ impl From for ClientTimeoutConfigBuilder { fn from(cfg: ClientTimeoutConfig) -> ClientTimeoutConfigBuilder { let mut builder = ClientTimeoutConfigBuilder::default(); builder - .stream_timeout(cfg.stream_timeout) + .connect_timeout(cfg.connect_timeout) .resolve_timeout(cfg.resolve_timeout) .resolve_ptr_timeout(cfg.resolve_ptr_timeout); @@ -125,7 +125,7 @@ impl ClientTimeoutConfig { } /// Return the default stream timeout -fn default_dns_stream_timeout() -> Duration { +fn default_connect_timeout() -> Duration { Duration::new(10, 0) } @@ -263,8 +263,8 @@ pub struct TorClientConfig { /// Rules about which addresses the client is willing to connect to. pub(crate) address_filter: ClientAddrConfig, - /// Rules about client DNS configuration - pub(crate) timeout_rules: ClientTimeoutConfig, + /// Information about timing out client requests. + pub(crate) stream_timeouts: ClientTimeoutConfig, } impl Default for TorClientConfig { @@ -326,8 +326,9 @@ pub struct TorClientConfigBuilder { circuit_timing: circ::CircuitTimingBuilder, /// Inner builder for the `address_filter` section. address_filter: ClientAddrConfigBuilder, - /// Inner builder for the `timeout_rules` section. - timeout_rules: ClientTimeoutConfigBuilder, + /// Inner builder for the `stream_timeouts + //` section. + stream_timeouts: ClientTimeoutConfigBuilder, } impl TorClientConfigBuilder { @@ -359,10 +360,10 @@ impl TorClientConfigBuilder { .address_filter .build() .map_err(|e| e.within("address_filter"))?; - let timeout_rules = self - .timeout_rules + let stream_timeouts = self + .stream_timeouts .build() - .map_err(|e| e.within("timeout_rules"))?; + .map_err(|e| e.within("stream_timeouts"))?; Ok(TorClientConfig { tor_network, @@ -373,7 +374,7 @@ impl TorClientConfigBuilder { preemptive_circuits, circuit_timing, address_filter, - timeout_rules, + stream_timeouts, }) } @@ -481,7 +482,7 @@ impl From for TorClientConfigBuilder { preemptive_circuits, circuit_timing, address_filter, - timeout_rules, + stream_timeouts, } = cfg; TorClientConfigBuilder { @@ -493,7 +494,7 @@ impl From for TorClientConfigBuilder { preemptive_circuits: preemptive_circuits.into(), circuit_timing: circuit_timing.into(), address_filter: address_filter.into(), - timeout_rules: timeout_rules.into(), + stream_timeouts: stream_timeouts.into(), } } } diff --git a/crates/arti-config/src/arti_defaults.toml b/crates/arti-config/src/arti_defaults.toml index dca04d60c..9ac6c67be 100644 --- a/crates/arti-config/src/arti_defaults.toml +++ b/crates/arti-config/src/arti_defaults.toml @@ -138,11 +138,15 @@ request_loyalty = "50 msec" # Should we allow attempts to make Tor connections to local addresses? allow_local_addrs = false -# Rules for a circuit timeouts -[timeout_rules] +# Rules for how long streams should wait when connecting to host or performing a +# DNS lookup. +# +# These timeouts measure the permitted time between sending a request on an +# established circuit, and getting a response from the exit node. +[stream_timeouts] # How long should we wait before timing out a stream when connecting to a host? -stream_timeout = "10 sec" +connect_timeout = "10 sec" # How long should we wait before timing out when resolving a DNS record? resolve_timeout = "10 sec" diff --git a/crates/arti-config/src/options.rs b/crates/arti-config/src/options.rs index 8077f1628..f63a3b28a 100644 --- a/crates/arti-config/src/options.rs +++ b/crates/arti-config/src/options.rs @@ -161,8 +161,8 @@ pub struct ArtiConfig { /// Rules about which addresses the client is willing to connect to. address_filter: ClientAddrConfig, - /// Rules about a client's DNS resolution. - timeout_rules: ClientTimeoutConfig, + /// Information about when to time out client requests. + stream_timeouts: ClientTimeoutConfig, } impl From for TorClientConfigBuilder { @@ -242,8 +242,8 @@ pub struct ArtiConfigBuilder { circuit_timing: circ::CircuitTimingBuilder, /// Builder for the address_filter section. address_filter: ClientAddrConfigBuilder, - /// Builder for the DNS resolution rules. - timeout_rules: ClientTimeoutConfigBuilder, + /// Builder for the stream timeout rules. + stream_timeouts: ClientTimeoutConfigBuilder, } impl ArtiConfigBuilder { @@ -277,10 +277,10 @@ impl ArtiConfigBuilder { .address_filter .build() .map_err(|e| e.within("address_filter"))?; - let timeout_rules = self - .timeout_rules + let stream_timeouts = self + .stream_timeouts .build() - .map_err(|e| e.within("timeout_rules"))?; + .map_err(|e| e.within("stream_timeouts"))?; Ok(ArtiConfig { proxy, logging, @@ -292,7 +292,7 @@ impl ArtiConfigBuilder { preemptive_circuits, circuit_timing, address_filter, - timeout_rules, + stream_timeouts, }) } @@ -389,8 +389,8 @@ impl ArtiConfigBuilder { /// /// This section controls how Arti should handle an exit relay's DNS /// resolution. - pub fn timeout_rules(&mut self) -> &mut ClientTimeoutConfigBuilder { - &mut self.timeout_rules + pub fn stream_timeouts(&mut self) -> &mut ClientTimeoutConfigBuilder { + &mut self.stream_timeouts } } @@ -407,7 +407,7 @@ impl From for ArtiConfigBuilder { preemptive_circuits: cfg.preemptive_circuits.into(), circuit_timing: cfg.circuit_timing.into(), address_filter: cfg.address_filter.into(), - timeout_rules: cfg.timeout_rules.into(), + stream_timeouts: cfg.stream_timeouts.into(), } } } From 2337d14ecffcba33f2c2175d3c254e478c73fae8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 7 Dec 2021 14:05:41 -0500 Subject: [PATCH 2/2] Make ClientTimeoutConfig members crate-private. We shouldn't have pub members in these config objects. --- crates/arti-client/src/config.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/arti-client/src/config.rs b/crates/arti-client/src/config.rs index 35062ccbc..70900883c 100644 --- a/crates/arti-client/src/config.rs +++ b/crates/arti-client/src/config.rs @@ -59,18 +59,18 @@ pub struct ClientTimeoutConfig { /// to a host? #[builder(default = "default_connect_timeout()")] #[serde(with = "humantime_serde", default = "default_connect_timeout")] - pub connect_timeout: Duration, + pub(crate) connect_timeout: Duration, /// How long should we wait before timing out when resolving a DNS record? #[builder(default = "default_dns_resolve_timeout()")] #[serde(with = "humantime_serde", default = "default_dns_resolve_timeout")] - pub resolve_timeout: Duration, + pub(crate) resolve_timeout: Duration, /// How long should we wait before timing out when resolving a DNS /// PTR record? #[builder(default = "default_dns_resolve_ptr_timeout()")] #[serde(with = "humantime_serde", default = "default_dns_resolve_ptr_timeout")] - pub resolve_ptr_timeout: Duration, + pub(crate) resolve_ptr_timeout: Duration, } // NOTE: it seems that `unwrap` may be safe because of builder defaults