From 312e415cd4067d51c92af20e9bcbb6fe878946b6 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Mon, 31 Jan 2022 14:25:25 +0000 Subject: [PATCH] timeout methods take Durations --- CHANGES.md | 5 + actix-http/CHANGES.md | 9 ++ actix-http/examples/bench.rs | 4 +- actix-http/examples/echo.rs | 6 +- actix-http/examples/hello-world.rs | 6 +- actix-http/src/builder.rs | 91 +++++++++-------- actix-http/src/config.rs | 139 +++++++++----------------- actix-http/src/h1/client.rs | 25 ++--- actix-http/src/h1/codec.rs | 2 +- actix-http/src/h1/dispatcher.rs | 10 +- actix-http/src/h1/dispatcher_tests.rs | 64 ++++++++++-- actix-http/src/h2/dispatcher.rs | 12 +-- actix-http/src/h2/mod.rs | 6 +- actix-http/src/keep_alive.rs | 72 +++++++++++++ actix-http/src/lib.rs | 4 +- actix-http/src/service.rs | 28 +++--- actix-http/tests/test_h2_timer.rs | 8 +- actix-http/tests/test_server.rs | 14 +-- actix-test/CHANGES.md | 3 + actix-test/src/lib.rs | 30 +++--- src/server.rs | 45 ++++++--- tests/test_httpserver.rs | 6 +- tests/test_server.rs | 8 +- 23 files changed, 356 insertions(+), 241 deletions(-) create mode 100644 actix-http/src/keep_alive.rs diff --git a/CHANGES.md b/CHANGES.md index 8c3997663..c00bc7198 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,10 +1,15 @@ # Changes ## Unreleased - 2021-xx-xx +### Changed +- Rename `HttpServer::{client_timeout => client_request_timeout}`. [#2611] +- Rename `HttpServer::{client_shutdown => client_disconnect_timeout}`. [#2611] + ### Removed - `impl Future for HttpResponse`. [#2601] [#2601]: https://github.com/actix/actix-web/pull/2601 +[#2611]: https://github.com/actix/actix-web/pull/2611 ## 4.0.0-beta.21 - 2022-01-21 diff --git a/actix-http/CHANGES.md b/actix-http/CHANGES.md index 4975dc5d6..d6d2555cb 100644 --- a/actix-http/CHANGES.md +++ b/actix-http/CHANGES.md @@ -3,16 +3,25 @@ ## Unreleased - 2021-xx-xx ### Added - Implement `Default` for `KeepAlive`. [#2611] +- Implement `From> for KeepAlive`. [#2611] ### Changed - Rename `ServiceConfig::{client_timer_expire => client_request_deadline}`. [#2611] - Rename `ServiceConfig::{client_timer => client_request_timer}`. [#2611] - Rename `ServiceConfig::{client_disconnect_timer => client_disconnect_deadline}`. [#2611] - Rename `ServiceConfig::{keep_alive_timer => keep_alive_deadline}`. [#2611] +- Deadline methods in `ServiceConfig` now return `std::time::Instant`s instead of Tokio's wrapper type. [#2611] +- Rename `h1::Codec::{keepalive => keep_alive}`. [#2611] +- Rename `h1::Codec::{keepalive_enabled => keep_alive_enabled}`. [#2611] +- `HttpServiceBuilder::keep_alive` now receives a `Duration` instead of an integer number of seconds. [#2611] ### Fixed - HTTP/1.1 dispatcher correctly uses client request timeout. [#2611] +### Removed +- `impl From for KeepAlive`; use `Duration`s instead. [#2611] +- `impl From> for KeepAlive`; use `Duration`s instead. [#2611] + [#2611]: https://github.com/actix/actix-web/pull/2611 diff --git a/actix-http/examples/bench.rs b/actix-http/examples/bench.rs index 063f9b938..e41c0bb4f 100644 --- a/actix-http/examples/bench.rs +++ b/actix-http/examples/bench.rs @@ -1,4 +1,4 @@ -use std::{convert::Infallible, io}; +use std::{convert::Infallible, io, time::Duration}; use actix_http::{HttpService, Request, Response, StatusCode}; use actix_server::Server; @@ -13,7 +13,7 @@ async fn main() -> io::Result<()> { Server::build() .bind("dispatcher-benchmark", ("127.0.0.1", 8080), || { HttpService::build() - .client_timeout(1000) + .client_request_timeout(Duration::from_secs(1)) .finish(|_: Request| async move { let mut res = Response::build(StatusCode::OK); Ok::<_, Infallible>(res.body(&**STR)) diff --git a/actix-http/examples/echo.rs b/actix-http/examples/echo.rs index f9188ed9f..58de64530 100644 --- a/actix-http/examples/echo.rs +++ b/actix-http/examples/echo.rs @@ -1,4 +1,4 @@ -use std::io; +use std::{io, time::Duration}; use actix_http::{Error, HttpService, Request, Response, StatusCode}; use actix_server::Server; @@ -13,8 +13,8 @@ async fn main() -> io::Result<()> { Server::build() .bind("echo", ("127.0.0.1", 8080), || { HttpService::build() - .client_timeout(1000) - .client_disconnect(1000) + .client_request_timeout(Duration::from_secs(1)) + .client_disconnect_timeout(Duration::from_secs(1)) // handles HTTP/1.1 and HTTP/2 .finish(|mut req: Request| async move { let mut body = BytesMut::new(); diff --git a/actix-http/examples/hello-world.rs b/actix-http/examples/hello-world.rs index a29903cc4..1a83d4d9c 100644 --- a/actix-http/examples/hello-world.rs +++ b/actix-http/examples/hello-world.rs @@ -1,4 +1,4 @@ -use std::{convert::Infallible, io}; +use std::{convert::Infallible, io, time::Duration}; use actix_http::{ header::HeaderValue, HttpMessage, HttpService, Request, Response, StatusCode, @@ -12,8 +12,8 @@ async fn main() -> io::Result<()> { Server::build() .bind("hello-world", ("127.0.0.1", 8080), || { HttpService::build() - .client_timeout(1000) - .client_disconnect(1000) + .client_request_timeout(Duration::from_secs(1)) + .client_disconnect_timeout(Duration::from_secs(1)) .on_connect_ext(|_, ext| { ext.insert(42u32); }) diff --git a/actix-http/src/builder.rs b/actix-http/src/builder.rs index e8a23a9e9..9dd145ce1 100644 --- a/actix-http/src/builder.rs +++ b/actix-http/src/builder.rs @@ -1,25 +1,23 @@ -use std::{fmt, marker::PhantomData, net, rc::Rc}; +use std::{fmt, marker::PhantomData, net, rc::Rc, time::Duration}; use actix_codec::Framed; use actix_service::{IntoServiceFactory, Service, ServiceFactory}; use crate::{ body::{BoxBody, MessageBody}, - config::{KeepAlive, ServiceConfig}, h1::{self, ExpectHandler, H1Service, UpgradeHandler}, h2::H2Service, service::HttpService, - ConnectCallback, Extensions, Request, Response, + ConnectCallback, Extensions, KeepAlive, Request, Response, ServiceConfig, }; -/// A HTTP service builder +/// An HTTP service builder. /// -/// This type can be used to construct an instance of [`HttpService`] through a -/// builder-like pattern. +/// This type can construct an instance of [`HttpService`] through a builder-like pattern. pub struct HttpServiceBuilder { keep_alive: KeepAlive, - client_timeout: u64, - client_disconnect: u64, + client_request_timeout: Duration, + client_disconnect_timeout: Duration, secure: bool, local_addr: Option, expect: X, @@ -28,22 +26,23 @@ pub struct HttpServiceBuilder { _phantom: PhantomData, } -impl HttpServiceBuilder +impl Default for HttpServiceBuilder where S: ServiceFactory, S::Error: Into> + 'static, S::InitError: fmt::Debug, >::Future: 'static, { - /// Create instance of `ServiceConfigBuilder` - #[allow(clippy::new_without_default)] - pub fn new() -> Self { + fn default() -> Self { HttpServiceBuilder { + // ServiceConfig parts (make sure defaults match) keep_alive: KeepAlive::default(), - client_timeout: 5000, - client_disconnect: 0, + client_request_timeout: Duration::from_secs(5), + client_disconnect_timeout: Duration::ZERO, secure: false, local_addr: None, + + // dispatcher parts expect: ExpectHandler, upgrade: None, on_connect_ext: None, @@ -65,9 +64,11 @@ where U::Error: fmt::Display, U::InitError: fmt::Debug, { - /// Set server keep-alive setting. + /// Set connection keep-alive setting. /// - /// By default keep alive is set to a 5 seconds. + /// Applies to HTTP/1.1 keep-alive and HTTP/2 ping-pong. + /// + /// By default keep-alive is 5 seconds. pub fn keep_alive>(mut self, val: W) -> Self { self.keep_alive = val.into(); self @@ -85,33 +86,45 @@ where self } - /// Set server client timeout in milliseconds for first request. + /// Set client request timeout (for first request). /// - /// Defines a timeout for reading client request header. If a client does not transmit - /// the entire set headers within this time, the request is terminated with - /// the 408 (Request Time-out) error. + /// Defines a timeout for reading client request header. If the client does not transmit the + /// request head within this duration, the connection is terminated with a `408 Request Timeout` + /// response error. /// - /// To disable timeout set value to 0. + /// A duration of zero disables the timeout. /// - /// By default client timeout is set to 5000 milliseconds. - pub fn client_timeout(mut self, val: u64) -> Self { - self.client_timeout = val; + /// By default, the client timeout is 5 seconds. + pub fn client_request_timeout(mut self, dur: Duration) -> Self { + self.client_request_timeout = dur; self } - /// Set server connection disconnect timeout in milliseconds. + #[doc(hidden)] + #[deprecated(since = "3.0.0", note = "Renamed to `client_request_timeout`.")] + pub fn client_timeout(self, dur: Duration) -> Self { + self.client_request_timeout(dur) + } + + /// Set client connection disconnect timeout. /// /// Defines a timeout for disconnect connection. If a disconnect procedure does not complete /// within this time, the request get dropped. This timeout affects secure connections. /// - /// To disable timeout set value to 0. + /// A duration of zero disables the timeout. /// - /// By default disconnect timeout is set to 0. - pub fn client_disconnect(mut self, val: u64) -> Self { - self.client_disconnect = val; + /// By default, the disconnect timeout is disabled. + pub fn client_disconnect_timeout(mut self, dur: Duration) -> Self { + self.client_disconnect_timeout = dur; self } + #[doc(hidden)] + #[deprecated(since = "3.0.0", note = "Renamed to `client_disconnect_timeout`.")] + pub fn client_disconnect(self, dur: Duration) -> Self { + self.client_disconnect_timeout(dur) + } + /// Provide service for `EXPECT: 100-Continue` support. /// /// Service get called with request that contains `EXPECT` header. @@ -126,8 +139,8 @@ where { HttpServiceBuilder { keep_alive: self.keep_alive, - client_timeout: self.client_timeout, - client_disconnect: self.client_disconnect, + client_request_timeout: self.client_request_timeout, + client_disconnect_timeout: self.client_disconnect_timeout, secure: self.secure, local_addr: self.local_addr, expect: expect.into_factory(), @@ -150,8 +163,8 @@ where { HttpServiceBuilder { keep_alive: self.keep_alive, - client_timeout: self.client_timeout, - client_disconnect: self.client_disconnect, + client_request_timeout: self.client_request_timeout, + client_disconnect_timeout: self.client_disconnect_timeout, secure: self.secure, local_addr: self.local_addr, expect: self.expect, @@ -185,8 +198,8 @@ where { let cfg = ServiceConfig::new( self.keep_alive, - self.client_timeout, - self.client_disconnect, + self.client_request_timeout, + self.client_disconnect_timeout, self.secure, self.local_addr, ); @@ -209,8 +222,8 @@ where { let cfg = ServiceConfig::new( self.keep_alive, - self.client_timeout, - self.client_disconnect, + self.client_request_timeout, + self.client_disconnect_timeout, self.secure, self.local_addr, ); @@ -230,8 +243,8 @@ where { let cfg = ServiceConfig::new( self.keep_alive, - self.client_timeout, - self.client_disconnect, + self.client_request_timeout, + self.client_disconnect_timeout, self.secure, self.local_addr, ); diff --git a/actix-http/src/config.rs b/actix-http/src/config.rs index d67f63fa5..efe75ca02 100644 --- a/actix-http/src/config.rs +++ b/actix-http/src/config.rs @@ -3,62 +3,26 @@ use std::{ fmt::{self, Write}, net, rc::Rc, - time::{Duration, SystemTime}, + time::{Duration, Instant, SystemTime}, }; -use actix_rt::{ - task::JoinHandle, - time::{interval, sleep_until, Instant, Sleep}, -}; +use actix_rt::{task::JoinHandle, time::interval}; use bytes::BytesMut; +use crate::KeepAlive; + /// "Thu, 01 Jan 1970 00:00:00 GMT".len() pub(crate) const DATE_VALUE_LENGTH: usize = 29; -#[derive(Debug, PartialEq, Clone, Copy)] -/// Server keep-alive setting -pub enum KeepAlive { - /// Keep-alive duration. - Timeout(Duration), - - /// Rely on OS to shutdown TCP connection. - Os, - - /// Keep-alive is disabled. - Disabled, -} - -impl Default for KeepAlive { - fn default() -> Self { - Self::Timeout(Duration::from_secs(5)) - } -} - -impl From for KeepAlive { - fn from(ka_secs: usize) -> Self { - KeepAlive::Timeout(Duration::from_secs(ka_secs as u64)) - } -} - -impl From> for KeepAlive { - fn from(ka_secs_opt: Option) -> Self { - match ka_secs_opt { - Some(ka_secs) => KeepAlive::Timeout(Duration::from_secs(ka_secs as u64)), - None => KeepAlive::Disabled, - } - } -} - /// HTTP service configuration. #[derive(Debug, Clone)] pub struct ServiceConfig(Rc); #[derive(Debug)] struct Inner { - keep_alive: Option, - client_request_timeout: u64, - client_disconnect_timeout: u64, - ka_enabled: bool, + keep_alive: KeepAlive, + client_request_timeout: Duration, + client_disconnect_timeout: Duration, secure: bool, local_addr: Option, date_service: DateService, @@ -66,7 +30,13 @@ struct Inner { impl Default for ServiceConfig { fn default() -> Self { - Self::new(KeepAlive::default(), 0, 0, false, None) + Self::new( + KeepAlive::default(), + Duration::from_secs(5), + Duration::ZERO, + false, + None, + ) } } @@ -74,22 +44,19 @@ impl ServiceConfig { /// Create instance of `ServiceConfig` pub fn new( keep_alive: KeepAlive, - client_request_timeout: u64, - client_disconnect_timeout: u64, + client_request_timeout: Duration, + client_disconnect_timeout: Duration, secure: bool, local_addr: Option, ) -> ServiceConfig { - let (keep_alive, ka_enabled) = match keep_alive { - KeepAlive::Timeout(val) => (val, true), - KeepAlive::Os => (Duration::ZERO, true), - KeepAlive::Disabled => (Duration::ZERO, false), + // zero timeout keep-alive maps to disabled + let keep_alive = match keep_alive { + KeepAlive::Timeout(Duration::ZERO) => KeepAlive::Disabled, + ka => ka, }; - let keep_alive = (ka_enabled && keep_alive > Duration::ZERO).then(|| keep_alive); - ServiceConfig(Rc::new(Inner { keep_alive, - ka_enabled, client_request_timeout, client_disconnect_timeout, secure, @@ -112,16 +79,22 @@ impl ServiceConfig { self.0.local_addr } - /// Keep-alive duration, if configured. + /// Connection keep-alive setting. #[inline] - pub fn keep_alive(&self) -> Option { + pub fn keep_alive(&self) -> KeepAlive { self.0.keep_alive } - /// Returns `true` if connection if set to use keep-alive functionality. - #[inline] - pub fn keep_alive_enabled(&self) -> bool { - self.0.ka_enabled + /// Creates a time object representing the deadline for this connection's keep-alive period, if + /// enabled. + /// + /// When [`KeepAlive::Os`] or [`KeepAlive::Disabled`] is set, this will return `None`. + pub fn keep_alive_deadline(&self) -> Option { + match self.keep_alive() { + KeepAlive::Timeout(dur) => Some(self.now() + dur), + KeepAlive::Os => None, + KeepAlive::Disabled => None, + } } /// Creates a time object representing the deadline for the client to finish sending the head of @@ -129,48 +102,26 @@ impl ServiceConfig { /// /// Returns `None` if this `ServiceConfig was` constructed with `client_request_timeout: 0`. pub fn client_request_deadline(&self) -> Option { - let delay = self.0.client_request_timeout; + let timeout = self.0.client_request_timeout; - if delay != 0 { - Some(self.now() + Duration::from_millis(delay)) + if timeout != Duration::ZERO { + Some(self.now() + timeout) } else { None } } - /// Creates a timer that resolves at the [client's first request deadline]. - /// - /// Returns `None` if this `ServiceConfig was` constructed with `client_request_timeout: 0`. - /// - /// [client request deadline]: Self::client_deadline - pub fn client_request_timer(&self) -> Option { - self.client_request_deadline().map(sleep_until) - } - /// Creates a time object representing the deadline for the client to disconnect. pub fn client_disconnect_deadline(&self) -> Option { - let delay = self.0.client_disconnect_timeout; + let timeout = self.0.client_disconnect_timeout; - if delay != 0 { - Some(self.now() + Duration::from_millis(delay)) + if timeout != Duration::ZERO { + Some(self.now() + timeout) } else { None } } - /// Creates a time object representing the deadline for the connection keep-alive, - /// if configured. - pub fn keep_alive_deadline(&self) -> Option { - self.keep_alive().map(|ka| self.now() + ka) - } - - /// Creates a timer that resolves at the [keep-alive deadline]. - /// - /// [keep-alive deadline]: Self::keep_alive_deadline - pub fn keep_alive_timer(&self) -> Option { - self.keep_alive_deadline().map(sleep_until) - } - pub(crate) fn now(&self) -> Instant { self.0.date_service.now() } @@ -188,7 +139,7 @@ impl ServiceConfig { dst.extend_from_slice(&buf); } - pub(crate) fn set_date_header(&self, dst: &mut BytesMut) { + pub(crate) fn write_date_header(&self, dst: &mut BytesMut) { self.0 .date_service .set_date(|date| dst.extend_from_slice(&date.bytes)); @@ -247,7 +198,7 @@ impl DateService { loop { let now = interval.tick().await; let date = Date::new(); - current_clone.set((date, now)); + current_clone.set((date, now.into_std())); } }); @@ -334,12 +285,16 @@ mod notify_on_drop { mod tests { use super::*; - use actix_rt::{task::yield_now, time::sleep}; + use actix_rt::{ + task::yield_now, + time::{sleep, sleep_until}, + }; use memchr::memmem; #[actix_rt::test] async fn test_date_service_update() { - let settings = ServiceConfig::new(KeepAlive::Os, 0, 0, false, None); + let settings = + ServiceConfig::new(KeepAlive::Os, Duration::ZERO, Duration::ZERO, false, None); yield_now().await; @@ -347,7 +302,7 @@ mod tests { settings.set_date(&mut buf1, false); let now1 = settings.now(); - sleep_until(Instant::now() + Duration::from_secs(2)).await; + sleep_until((Instant::now() + Duration::from_secs(2)).into()).await; yield_now().await; let now2 = settings.now(); diff --git a/actix-http/src/h1/client.rs b/actix-http/src/h1/client.rs index 9bd896ae0..fdb8e184c 100644 --- a/actix-http/src/h1/client.rs +++ b/actix-http/src/h1/client.rs @@ -38,7 +38,7 @@ struct ClientCodecInner { decoder: decoder::MessageDecoder, payload: Option, version: Version, - ctype: ConnectionType, + conn_type: ConnectionType, // encoder part flags: Flags, @@ -56,18 +56,19 @@ impl ClientCodec { /// /// `keepalive_enabled` how response `connection` header get generated. pub fn new(config: ServiceConfig) -> Self { - let flags = if config.keep_alive_enabled() { + let flags = if config.keep_alive().enabled() { Flags::KEEPALIVE_ENABLED } else { Flags::empty() }; + ClientCodec { inner: ClientCodecInner { config, decoder: decoder::MessageDecoder::default(), payload: None, version: Version::HTTP_11, - ctype: ConnectionType::Close, + conn_type: ConnectionType::Close, flags, encoder: encoder::MessageEncoder::default(), @@ -77,12 +78,12 @@ impl ClientCodec { /// Check if request is upgrade pub fn upgrade(&self) -> bool { - self.inner.ctype == ConnectionType::Upgrade + self.inner.conn_type == ConnectionType::Upgrade } /// Check if last response is keep-alive pub fn keepalive(&self) -> bool { - self.inner.ctype == ConnectionType::KeepAlive + self.inner.conn_type == ConnectionType::KeepAlive } /// Check last request's message type @@ -105,7 +106,7 @@ impl ClientCodec { impl ClientPayloadCodec { /// Check if last response is keep-alive pub fn keepalive(&self) -> bool { - self.inner.ctype == ConnectionType::KeepAlive + self.inner.conn_type == ConnectionType::KeepAlive } /// Transform payload codec to a message codec @@ -122,12 +123,12 @@ impl Decoder for ClientCodec { debug_assert!(!self.inner.payload.is_some(), "Payload decoder is set"); if let Some((req, payload)) = self.inner.decoder.decode(src)? { - if let Some(ctype) = req.conn_type() { + if let Some(conn_type) = req.conn_type() { // do not use peer's keep-alive - self.inner.ctype = if ctype == ConnectionType::KeepAlive { - self.inner.ctype + self.inner.conn_type = if conn_type == ConnectionType::KeepAlive { + self.inner.conn_type } else { - ctype + conn_type }; } @@ -192,7 +193,7 @@ impl Encoder> for ClientCodec { .set(Flags::HEAD, head.as_ref().method == Method::HEAD); // connection status - inner.ctype = match head.as_ref().connection_type() { + inner.conn_type = match head.as_ref().connection_type() { ConnectionType::KeepAlive => { if inner.flags.contains(Flags::KEEPALIVE_ENABLED) { ConnectionType::KeepAlive @@ -211,7 +212,7 @@ impl Encoder> for ClientCodec { false, inner.version, length, - inner.ctype, + inner.conn_type, &inner.config, )?; } diff --git a/actix-http/src/h1/codec.rs b/actix-http/src/h1/codec.rs index d2da00abc..f1ef9a54e 100644 --- a/actix-http/src/h1/codec.rs +++ b/actix-http/src/h1/codec.rs @@ -51,7 +51,7 @@ impl Codec { /// /// `keepalive_enabled` how response `connection` header get generated. pub fn new(config: ServiceConfig) -> Self { - let flags = if config.keep_alive_enabled() { + let flags = if config.keep_alive().enabled() { Flags::KEEP_ALIVE_ENABLED } else { Flags::empty() diff --git a/actix-http/src/h1/dispatcher.rs b/actix-http/src/h1/dispatcher.rs index 6b14b39ee..1fa357713 100644 --- a/actix-http/src/h1/dispatcher.rs +++ b/actix-http/src/h1/dispatcher.rs @@ -264,7 +264,7 @@ where messages: VecDeque::new(), head_timer: TimerState::new(config.client_request_deadline().is_some()), - ka_timer: TimerState::new(config.keep_alive_enabled()), + ka_timer: TimerState::new(config.keep_alive().enabled()), shutdown_timer: TimerState::new( config.client_disconnect_deadline().is_some(), ), @@ -876,7 +876,7 @@ where if let Some(deadline) = this.config.client_disconnect_deadline() { // start shutdown timeout if enabled this.shutdown_timer - .set_and_init(cx, sleep_until(deadline), line!()); + .set_and_init(cx, sleep_until(deadline.into()), line!()); } else { // no shutdown timeout, drop socket this.flags.insert(Flags::WRITE_DISCONNECT); @@ -1100,7 +1100,7 @@ where if let Some(deadline) = inner.config.client_request_deadline() { inner.as_mut().project().head_timer.set_and_init( cx, - sleep_until(deadline), + sleep_until(deadline.into()), line!(), ); } @@ -1125,10 +1125,10 @@ where PollResponse::DoNothing => { if inner.flags.contains(Flags::FINISHED | Flags::KEEP_ALIVE) { - if let Some(timer) = inner.config.keep_alive_timer() { + if let Some(timer) = inner.config.keep_alive_deadline() { inner.as_mut().project().ka_timer.set_and_init( cx, - timer, + sleep_until(timer.into()), line!(), ); } diff --git a/actix-http/src/h1/dispatcher_tests.rs b/actix-http/src/h1/dispatcher_tests.rs index 2fc306290..891cce69c 100644 --- a/actix-http/src/h1/dispatcher_tests.rs +++ b/actix-http/src/h1/dispatcher_tests.rs @@ -73,7 +73,13 @@ fn echo_payload_service() -> impl Service, E async fn late_request() { let mut buf = TestBuffer::empty(); - let cfg = ServiceConfig::new(KeepAlive::Disabled, 100, 0, false, None); + let cfg = ServiceConfig::new( + KeepAlive::Disabled, + Duration::from_millis(100), + Duration::ZERO, + false, + None, + ); let services = HttpFlow::new(ok_service(), ExpectHandler, None); let h1 = Dispatcher::<_, _, _, _, UpgradeHandler>::new( @@ -134,7 +140,13 @@ async fn late_request() { async fn oneshot_connection() { let buf = TestBuffer::new("GET /abcd HTTP/1.1\r\n\r\n"); - let cfg = ServiceConfig::new(KeepAlive::Disabled, 100, 0, false, None); + let cfg = ServiceConfig::new( + KeepAlive::Disabled, + Duration::from_millis(100), + Duration::ZERO, + false, + None, + ); let services = HttpFlow::new(echo_path_service(), ExpectHandler, None); let h1 = Dispatcher::<_, _, _, _, UpgradeHandler>::new( @@ -188,8 +200,8 @@ async fn keep_alive_timeout() { let cfg = ServiceConfig::new( KeepAlive::Timeout(Duration::from_millis(200)), - 100, - 0, + Duration::from_millis(100), + Duration::ZERO, false, None, ); @@ -267,8 +279,8 @@ async fn keep_alive_follow_up_req() { let cfg = ServiceConfig::new( KeepAlive::Timeout(Duration::from_millis(500)), - 100, - 0, + Duration::from_millis(100), + Duration::ZERO, false, None, ); @@ -429,7 +441,13 @@ async fn pipelining_ok_then_ok() { ", ); - let cfg = ServiceConfig::new(KeepAlive::Disabled, 1, 1, false, None); + let cfg = ServiceConfig::new( + KeepAlive::Disabled, + Duration::from_millis(1), + Duration::from_millis(1), + false, + None, + ); let services = HttpFlow::new(echo_path_service(), ExpectHandler, None); @@ -493,7 +511,13 @@ async fn pipelining_ok_then_bad() { ", ); - let cfg = ServiceConfig::new(KeepAlive::Disabled, 1, 1, false, None); + let cfg = ServiceConfig::new( + KeepAlive::Disabled, + Duration::from_millis(1), + Duration::from_millis(1), + false, + None, + ); let services = HttpFlow::new(echo_path_service(), ExpectHandler, None); @@ -550,7 +574,13 @@ async fn pipelining_ok_then_bad() { async fn expect_handling() { lazy(|cx| { let mut buf = TestSeqBuffer::empty(); - let cfg = ServiceConfig::new(KeepAlive::Disabled, 0, 0, false, None); + let cfg = ServiceConfig::new( + KeepAlive::Disabled, + Duration::ZERO, + Duration::ZERO, + false, + None, + ); let services = HttpFlow::new(echo_payload_service(), ExpectHandler, None); @@ -621,7 +651,13 @@ async fn expect_handling() { async fn expect_eager() { lazy(|cx| { let mut buf = TestSeqBuffer::empty(); - let cfg = ServiceConfig::new(KeepAlive::Disabled, 0, 0, false, None); + let cfg = ServiceConfig::new( + KeepAlive::Disabled, + Duration::ZERO, + Duration::ZERO, + false, + None, + ); let services = HttpFlow::new(echo_path_service(), ExpectHandler, None); @@ -698,7 +734,13 @@ async fn upgrade_handling() { lazy(|cx| { let mut buf = TestSeqBuffer::empty(); - let cfg = ServiceConfig::new(KeepAlive::Disabled, 0, 0, false, None); + let cfg = ServiceConfig::new( + KeepAlive::Disabled, + Duration::ZERO, + Duration::ZERO, + false, + None, + ); let services = HttpFlow::new(ok_service(), ExpectHandler, Some(TestUpgrade)); diff --git a/actix-http/src/h2/dispatcher.rs b/actix-http/src/h2/dispatcher.rs index 5472ebb7a..b41a95600 100644 --- a/actix-http/src/h2/dispatcher.rs +++ b/actix-http/src/h2/dispatcher.rs @@ -57,11 +57,11 @@ where conn_data: OnConnectData, timer: Option>>, ) -> Self { - let ping_pong = config.keep_alive().map(|dur| H2PingPong { + let ping_pong = config.keep_alive().duration().map(|dur| H2PingPong { timer: timer .map(|mut timer| { - // reset timer if it's received from new function. - timer.as_mut().reset(config.now() + dur); + // reuse timer slot if it was used for handshake + timer.as_mut().reset((config.now() + dur).into()); timer }) .unwrap_or_else(|| Box::pin(sleep(dur))), @@ -161,7 +161,7 @@ where ping_pong.on_flight = false; let dead_line = this.config.keep_alive_deadline().unwrap(); - ping_pong.timer.as_mut().reset(dead_line); + ping_pong.timer.as_mut().reset(dead_line.into()); } Poll::Pending => { return ping_pong.timer.as_mut().poll(cx).map(|_| Ok(())) @@ -175,7 +175,7 @@ where ping_pong.ping_pong.send_ping(Ping::opaque())?; let dead_line = this.config.keep_alive_deadline().unwrap(); - ping_pong.timer.as_mut().reset(dead_line); + ping_pong.timer.as_mut().reset(dead_line.into()); ping_pong.on_flight = true; } @@ -322,7 +322,7 @@ fn prepare_response( // set date header if !has_date { let mut bytes = BytesMut::with_capacity(29); - config.set_date_header(&mut bytes); + config.write_date_header(&mut bytes); res.headers_mut().insert( DATE, // SAFETY: serialized date-times are known ASCII strings diff --git a/actix-http/src/h2/mod.rs b/actix-http/src/h2/mod.rs index a014969d0..c8aaaaa5f 100644 --- a/actix-http/src/h2/mod.rs +++ b/actix-http/src/h2/mod.rs @@ -7,7 +7,7 @@ use std::{ }; use actix_codec::{AsyncRead, AsyncWrite}; -use actix_rt::time::Sleep; +use actix_rt::time::{sleep_until, Sleep}; use bytes::Bytes; use futures_core::{ready, Stream}; use h2::{ @@ -67,7 +67,9 @@ where { HandshakeWithTimeout { handshake: handshake(io), - timer: config.client_request_timer().map(Box::pin), + timer: config + .client_request_deadline() + .map(|deadline| Box::pin(sleep_until(deadline.into()))), } } diff --git a/actix-http/src/keep_alive.rs b/actix-http/src/keep_alive.rs new file mode 100644 index 000000000..2f6ba2c6e --- /dev/null +++ b/actix-http/src/keep_alive.rs @@ -0,0 +1,72 @@ +use std::time::Duration; + +/// Connection keep-alive config. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum KeepAlive { + /// Keep-alive duration. + /// + /// `KeepAlive::Timeout(Duration::ZERO)` is mapped to `KeepAlive::Disabled`. + Timeout(Duration), + + /// Rely on OS to shutdown TCP connection. + /// + /// Some defaults can be very long, check your OS documentation. + Os, + + /// Keep-alive is disabled. + /// + /// Connections will be closed immediately. + Disabled, +} + +impl KeepAlive { + pub(crate) fn enabled(&self) -> bool { + matches!(self, Self::Timeout(_) | Self::Os) + } + + pub(crate) fn duration(&self) -> Option { + match self { + KeepAlive::Timeout(dur) => Some(*dur), + _ => None, + } + } +} + +impl Default for KeepAlive { + fn default() -> Self { + Self::Timeout(Duration::from_secs(5)) + } +} + +impl From for KeepAlive { + fn from(dur: Duration) -> Self { + KeepAlive::Timeout(dur) + } +} + +impl From> for KeepAlive { + fn from(ka_dur: Option) -> Self { + match ka_dur { + Some(Duration::ZERO) => KeepAlive::Disabled, + Some(dur) => KeepAlive::Timeout(dur), + None => KeepAlive::Disabled, + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn from_impls() { + let test: KeepAlive = Duration::from_secs(1).into(); + assert_eq!(test, KeepAlive::Timeout(Duration::from_secs(1))); + + let test: KeepAlive = Duration::from_secs(0).into(); + assert_eq!(test, KeepAlive::Disabled); + + let test: KeepAlive = None.into(); + assert_eq!(test, KeepAlive::Disabled); + } +} diff --git a/actix-http/src/lib.rs b/actix-http/src/lib.rs index f2b415790..2f469fe84 100644 --- a/actix-http/src/lib.rs +++ b/actix-http/src/lib.rs @@ -42,6 +42,7 @@ pub mod h2; pub mod header; mod helpers; mod http_message; +mod keep_alive; mod message; mod payload; mod requests; @@ -51,11 +52,12 @@ pub mod test; pub mod ws; pub use self::builder::HttpServiceBuilder; -pub use self::config::{KeepAlive, ServiceConfig}; +pub use self::config::ServiceConfig; pub use self::error::Error; pub use self::extensions::Extensions; pub use self::header::ContentEncoding; pub use self::http_message::HttpMessage; +pub use self::keep_alive::KeepAlive; pub use self::message::ConnectionType; pub use self::message::Message; #[allow(deprecated)] diff --git a/actix-http/src/service.rs b/actix-http/src/service.rs index 029fa3672..f650a848f 100644 --- a/actix-http/src/service.rs +++ b/actix-http/src/service.rs @@ -19,9 +19,8 @@ use pin_project_lite::pin_project; use crate::{ body::{BoxBody, MessageBody}, builder::HttpServiceBuilder, - config::{KeepAlive, ServiceConfig}, error::DispatchError, - h1, h2, ConnectCallback, OnConnectData, Protocol, Request, Response, + h1, h2, ConnectCallback, OnConnectData, Protocol, Request, Response, ServiceConfig, }; /// A `ServiceFactory` for HTTP/1.1 or HTTP/2 protocol. @@ -43,9 +42,9 @@ where >::Future: 'static, B: MessageBody + 'static, { - /// Create builder for `HttpService` instance. + /// Constructs builder for `HttpService` instance. pub fn build() -> HttpServiceBuilder { - HttpServiceBuilder::new() + HttpServiceBuilder::default() } } @@ -58,12 +57,10 @@ where >::Future: 'static, B: MessageBody + 'static, { - /// Create new `HttpService` instance. + /// Constructs new `HttpService` instance from service with default config. pub fn new>(service: F) -> Self { - let cfg = ServiceConfig::new(KeepAlive::default(), 5000, 0, false, None); - HttpService { - cfg, + cfg: ServiceConfig::default(), srv: service.into_factory(), expect: h1::ExpectHandler, upgrade: None, @@ -72,7 +69,7 @@ where } } - /// Create new `HttpService` instance with config. + /// Constructs new `HttpService` instance from config and service. pub(crate) fn with_config>( cfg: ServiceConfig, service: F, @@ -97,11 +94,10 @@ where >::Future: 'static, B: MessageBody, { - /// Provide service for `EXPECT: 100-Continue` support. + /// Sets service for `Expect: 100-Continue` handling. /// - /// Service get called with request that contains `EXPECT` header. - /// Service must return request in case of success, in that case - /// request will be forwarded to main service. + /// AN expect service is called with the request that contains an `Expect` header. A successful + /// response type is also a request which will be forwarded to the main service. pub fn expect(self, expect: X1) -> HttpService where X1: ServiceFactory, @@ -118,10 +114,10 @@ where } } - /// Provide service for custom `Connection: UPGRADE` support. + /// Sets service for custom `Connection: Upgrade` handling. /// - /// If service is provided then normal requests handling get halted - /// and this service get called with original request and framed object. + /// If service is provided then normal requests handling get halted and this service get called + /// with original request and framed object. pub fn upgrade(self, upgrade: Option) -> HttpService where U1: ServiceFactory<(Request, Framed), Config = (), Response = ()>, diff --git a/actix-http/tests/test_h2_timer.rs b/actix-http/tests/test_h2_timer.rs index 2b9c26e4a..2e1480297 100644 --- a/actix-http/tests/test_h2_timer.rs +++ b/actix-http/tests/test_h2_timer.rs @@ -1,4 +1,4 @@ -use std::io; +use std::{io, time::Duration}; use actix_http::{error::Error, HttpService, Response}; use actix_server::Server; @@ -19,7 +19,7 @@ async fn h2_ping_pong() -> io::Result<()> { .workers(1) .listen("h2_ping_pong", lst, || { HttpService::build() - .keep_alive(3) + .keep_alive(Duration::from_secs(3)) .h2(|_| async { Ok::<_, Error>(Response::ok()) }) .tcp() })? @@ -92,10 +92,10 @@ async fn h2_handshake_timeout() -> io::Result<()> { .workers(1) .listen("h2_ping_pong", lst, || { HttpService::build() - .keep_alive(30) + .keep_alive(Duration::from_secs(30)) // set first request timeout to 5 seconds. // this is the timeout used for http2 handshake. - .client_timeout(5000) + .client_request_timeout(Duration::from_secs(5)) .h2(|_| async { Ok::<_, Error>(Response::ok()) }) .tcp() })? diff --git a/actix-http/tests/test_server.rs b/actix-http/tests/test_server.rs index 33272453a..1b5de3425 100644 --- a/actix-http/tests/test_server.rs +++ b/actix-http/tests/test_server.rs @@ -26,8 +26,8 @@ async fn h1_basic() { let mut srv = test_server(|| { HttpService::build() .keep_alive(KeepAlive::Disabled) - .client_timeout(1000) - .client_disconnect(1000) + .client_request_timeout(Duration::from_secs(1)) + .client_disconnect_timeout(Duration::from_secs(1)) .h1(|req: Request| { assert!(req.peer_addr().is_some()); ok::<_, Infallible>(Response::ok()) @@ -47,8 +47,8 @@ async fn h1_2() { let mut srv = test_server(|| { HttpService::build() .keep_alive(KeepAlive::Disabled) - .client_timeout(1000) - .client_disconnect(1000) + .client_request_timeout(Duration::from_secs(1)) + .client_disconnect_timeout(Duration::from_secs(1)) .finish(|req: Request| { assert!(req.peer_addr().is_some()); assert_eq!(req.version(), http::Version::HTTP_11); @@ -200,8 +200,8 @@ async fn chunked_payload() { async fn slow_request_408() { let mut srv = test_server(|| { HttpService::build() - .client_timeout(200) - .keep_alive(2) + .client_request_timeout(Duration::from_millis(200)) + .keep_alive(Duration::from_secs(2)) .finish(|_| ok::<_, Infallible>(Response::ok())) .tcp() }) @@ -277,7 +277,7 @@ async fn http1_keepalive() { async fn http1_keepalive_timeout() { let mut srv = test_server(|| { HttpService::build() - .keep_alive(1) + .keep_alive(Duration::from_secs(1)) .h1(|_| ok::<_, Infallible>(Response::ok())) .tcp() }) diff --git a/actix-test/CHANGES.md b/actix-test/CHANGES.md index 32ab2344f..3877f4fbf 100644 --- a/actix-test/CHANGES.md +++ b/actix-test/CHANGES.md @@ -1,6 +1,9 @@ # Changes ## Unreleased - 2021-xx-xx +- Rename `TestServerConfig::{client_timeout => client_request_timeout}`. [#2611] + +[#2611]: https://github.com/actix/actix-web/pull/2611 ## 0.1.0-beta.11 - 2022-01-04 diff --git a/actix-test/src/lib.rs b/actix-test/src/lib.rs index f86120f2f..d44bc7a45 100644 --- a/actix-test/src/lib.rs +++ b/actix-test/src/lib.rs @@ -149,7 +149,7 @@ where let local_addr = tcp.local_addr().unwrap(); let factory = factory.clone(); let srv_cfg = cfg.clone(); - let timeout = cfg.client_timeout; + let timeout = cfg.client_request_timeout; let builder = Server::build().workers(1).disable_signals().system_exit(); @@ -167,7 +167,7 @@ where .map_err(|err| err.into().error_response()); HttpService::build() - .client_timeout(timeout) + .client_request_timeout(timeout) .h1(map_config(fac, move |_| app_cfg.clone())) .tcp() }), @@ -183,7 +183,7 @@ where .map_err(|err| err.into().error_response()); HttpService::build() - .client_timeout(timeout) + .client_request_timeout(timeout) .h2(map_config(fac, move |_| app_cfg.clone())) .tcp() }), @@ -199,7 +199,7 @@ where .map_err(|err| err.into().error_response()); HttpService::build() - .client_timeout(timeout) + .client_request_timeout(timeout) .finish(map_config(fac, move |_| app_cfg.clone())) .tcp() }), @@ -218,7 +218,7 @@ where .map_err(|err| err.into().error_response()); HttpService::build() - .client_timeout(timeout) + .client_request_timeout(timeout) .h1(map_config(fac, move |_| app_cfg.clone())) .openssl(acceptor.clone()) }), @@ -234,7 +234,7 @@ where .map_err(|err| err.into().error_response()); HttpService::build() - .client_timeout(timeout) + .client_request_timeout(timeout) .h2(map_config(fac, move |_| app_cfg.clone())) .openssl(acceptor.clone()) }), @@ -250,7 +250,7 @@ where .map_err(|err| err.into().error_response()); HttpService::build() - .client_timeout(timeout) + .client_request_timeout(timeout) .finish(map_config(fac, move |_| app_cfg.clone())) .openssl(acceptor.clone()) }), @@ -269,7 +269,7 @@ where .map_err(|err| err.into().error_response()); HttpService::build() - .client_timeout(timeout) + .client_request_timeout(timeout) .h1(map_config(fac, move |_| app_cfg.clone())) .rustls(config.clone()) }), @@ -285,7 +285,7 @@ where .map_err(|err| err.into().error_response()); HttpService::build() - .client_timeout(timeout) + .client_request_timeout(timeout) .h2(map_config(fac, move |_| app_cfg.clone())) .rustls(config.clone()) }), @@ -301,7 +301,7 @@ where .map_err(|err| err.into().error_response()); HttpService::build() - .client_timeout(timeout) + .client_request_timeout(timeout) .finish(map_config(fac, move |_| app_cfg.clone())) .rustls(config.clone()) }), @@ -388,7 +388,7 @@ pub fn config() -> TestServerConfig { pub struct TestServerConfig { tp: HttpVer, stream: StreamType, - client_timeout: u64, + client_request_timeout: Duration, } impl Default for TestServerConfig { @@ -403,7 +403,7 @@ impl TestServerConfig { TestServerConfig { tp: HttpVer::Both, stream: StreamType::Tcp, - client_timeout: 5000, + client_request_timeout: Duration::from_secs(5), } } @@ -433,9 +433,9 @@ impl TestServerConfig { self } - /// Set client timeout in milliseconds for first request. - pub fn client_timeout(mut self, val: u64) -> Self { - self.client_timeout = val; + /// Set client timeout for first request. + pub fn client_request_timeout(mut self, dur: Duration) -> Self { + self.client_request_timeout = dur; self } } diff --git a/src/server.rs b/src/server.rs index a318bbda1..1e9740fb6 100644 --- a/src/server.rs +++ b/src/server.rs @@ -4,6 +4,7 @@ use std::{ marker::PhantomData, net, sync::{Arc, Mutex}, + time::Duration, }; use actix_http::{body::MessageBody, Extensions, HttpService, KeepAlive, Request, Response}; @@ -27,8 +28,8 @@ struct Socket { struct Config { host: Option, keep_alive: KeepAlive, - client_timeout: u64, - client_shutdown: u64, + client_request_timeout: Duration, + client_disconnect_timeout: Duration, } /// An HTTP Server. @@ -89,8 +90,8 @@ where config: Arc::new(Mutex::new(Config { host: None, keep_alive: KeepAlive::default(), - client_timeout: 5000, - client_shutdown: 5000, + client_request_timeout: Duration::from_secs(5), + client_disconnect_timeout: Duration::from_secs(1), })), backlog: 1024, sockets: Vec::new(), @@ -200,11 +201,17 @@ where /// To disable timeout set value to 0. /// /// By default client timeout is set to 5000 milliseconds. - pub fn client_timeout(self, val: u64) -> Self { - self.config.lock().unwrap().client_timeout = val; + pub fn client_request_timeout(self, dur: Duration) -> Self { + self.config.lock().unwrap().client_request_timeout = dur; self } + #[doc(hidden)] + #[deprecated(since = "4.0.0", note = "Renamed to `client_request_timeout`.")] + pub fn client_timeout(self, dur: Duration) -> Self { + self.client_request_timeout(dur) + } + /// Set server connection shutdown timeout in milliseconds. /// /// Defines a timeout for shutdown connection. If a shutdown procedure does not complete @@ -213,11 +220,17 @@ where /// To disable timeout set value to 0. /// /// By default client timeout is set to 5000 milliseconds. - pub fn client_shutdown(self, val: u64) -> Self { - self.config.lock().unwrap().client_shutdown = val; + pub fn client_disconnect_timeout(self, dur: Duration) -> Self { + self.config.lock().unwrap().client_disconnect_timeout = dur; self } + #[doc(hidden)] + #[deprecated(since = "4.0.0", note = "Renamed to `client_request_timeout`.")] + pub fn client_shutdown(self, dur: u64) -> Self { + self.client_disconnect_timeout(Duration::from_millis(dur)) + } + /// Set server host name. /// /// Host name is used by application router as a hostname for url generation. @@ -291,8 +304,8 @@ where let mut svc = HttpService::build() .keep_alive(c.keep_alive) - .client_timeout(c.client_timeout) - .client_disconnect(c.client_shutdown) + .client_request_timeout(c.client_request_timeout) + .client_disconnect_timeout(c.client_disconnect_timeout) .local_addr(addr); if let Some(handler) = on_connect_fn.clone() { @@ -349,8 +362,8 @@ where let svc = HttpService::build() .keep_alive(c.keep_alive) - .client_timeout(c.client_timeout) - .client_disconnect(c.client_shutdown) + .client_request_timeout(c.client_request_timeout) + .client_disconnect_timeout(c.client_disconnect_timeout) .local_addr(addr); let svc = if let Some(handler) = on_connect_fn.clone() { @@ -537,8 +550,8 @@ where fn_service(|io: UnixStream| async { Ok((io, Protocol::Http1, None)) }).and_then({ let mut svc = HttpService::build() .keep_alive(c.keep_alive) - .client_timeout(c.client_timeout) - .client_disconnect(c.client_shutdown); + .client_request_timeout(c.client_request_timeout) + .client_disconnect_timeout(c.client_disconnect_timeout); if let Some(handler) = on_connect_fn.clone() { svc = svc @@ -593,8 +606,8 @@ where fn_service(|io: UnixStream| async { Ok((io, Protocol::Http1, None)) }).and_then( HttpService::build() .keep_alive(c.keep_alive) - .client_timeout(c.client_timeout) - .client_disconnect(c.client_shutdown) + .client_request_timeout(c.client_request_timeout) + .client_disconnect_timeout(c.client_disconnect_timeout) .finish(map_config(fac, move |_| config.clone())), ) }, diff --git a/tests/test_httpserver.rs b/tests/test_httpserver.rs index 6ea8e520c..86e0575f3 100644 --- a/tests/test_httpserver.rs +++ b/tests/test_httpserver.rs @@ -26,9 +26,9 @@ async fn test_start() { .backlog(1) .max_connections(10) .max_connection_rate(10) - .keep_alive(10) - .client_timeout(5000) - .client_shutdown(0) + .keep_alive(Duration::from_secs(10)) + .client_request_timeout(Duration::from_secs(5)) + .client_disconnect_timeout(Duration::ZERO) .server_hostname("localhost") .system_exit() .disable_signals() diff --git a/tests/test_server.rs b/tests/test_server.rs index b8193a004..bd8934061 100644 --- a/tests/test_server.rs +++ b/tests/test_server.rs @@ -8,6 +8,7 @@ use std::{ io::{Read, Write}, pin::Pin, task::{Context, Poll}, + time::Duration, }; use actix_web::{ @@ -835,9 +836,10 @@ async fn test_server_cookies() { async fn test_slow_request() { use std::net; - let srv = actix_test::start_with(actix_test::config().client_timeout(200), || { - App::new().service(web::resource("/").route(web::to(HttpResponse::Ok))) - }); + let srv = actix_test::start_with( + actix_test::config().client_request_timeout(Duration::from_millis(200)), + || App::new().service(web::resource("/").route(web::to(HttpResponse::Ok))), + ); let mut stream = net::TcpStream::connect(srv.addr()).unwrap(); let mut data = String::new();