Merge branch 'master' into multipart/fix/hang

Cargo.toml

@@ -97,6 +97,7 @@ log = "0.4"
 mime = "0.3"
 paste = "1"
 pin-project = "1.0.0"
+pin-project-lite = "0.2.7"
 regex = "1.4"
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0"

actix-http/CHANGES.md

@@ -1,6 +1,11 @@
 # Changes
 
 ## Unreleased - 2021-xx-xx
+### Changed
+* Guarantee ordering of `header::GetAll` iterator to be same as insertion order. [#2467]
+* Expose `header::{GetAll, Removed}` iterators. [#2467]
+
+[#2467]: https://github.com/actix/actix-web/pull/2467
 
 
 ## 3.0.0-beta.13 - 2021-11-22

actix-http/src/header/map.rs

@@ -288,7 +288,7 @@ impl HeaderMap {
     /// Returns an iterator over all values associated with a header name.
     ///
     /// The returned iterator does not incur any allocations and will yield no items if there are no
-    /// values associated with the key. Iteration order is **not** guaranteed to be the same as
+    /// values associated with the key. Iteration order is guaranteed to be the same as
     /// insertion order.
     ///
     /// # Examples
@@ -355,6 +355,19 @@ impl HeaderMap {
     ///
     /// assert_eq!(map.len(), 1);
     /// ```
+    ///
+    /// A convenience method is provided on the returned iterator to check if the insertion replaced
+    /// any values.
+    /// ```
+    /// # use actix_http::http::{header, HeaderMap, HeaderValue};
+    /// let mut map = HeaderMap::new();
+    ///
+    /// let removed = map.insert(header::ACCEPT, HeaderValue::from_static("text/plain"));
+    /// assert!(removed.is_empty());
+    ///
+    /// let removed = map.insert(header::ACCEPT, HeaderValue::from_static("text/html"));
+    /// assert!(!removed.is_empty());
+    /// ```
     pub fn insert(&mut self, key: HeaderName, val: HeaderValue) -> Removed {
         let value = self.inner.insert(key, Value::one(val));
         Removed::new(value)
@@ -393,6 +406,9 @@ impl HeaderMap {
 
     /// Removes all headers for a particular header name from the map.
     ///
+    /// Providing an invalid header names (as a string argument) will have no effect and return
+    /// without error.
+    ///
     /// # Examples
     /// ```
     /// # use actix_http::http::{header, HeaderMap, HeaderValue};
@@ -409,6 +425,21 @@ impl HeaderMap {
     /// assert!(removed.next().is_none());
     ///
     /// assert!(map.is_empty());
+    /// ```
+    ///
+    /// A convenience method is provided on the returned iterator to check if the `remove` call
+    /// actually removed any values.
+    /// ```
+    /// # use actix_http::http::{header, HeaderMap, HeaderValue};
+    /// let mut map = HeaderMap::new();
+    ///
+    /// let removed = map.remove("accept");
+    /// assert!(removed.is_empty());
+    ///
+    /// map.insert(header::ACCEPT, HeaderValue::from_static("text/html"));
+    /// let removed = map.remove("accept");
+    /// assert!(!removed.is_empty());
+    /// ```
     pub fn remove(&mut self, key: impl AsHeaderName) -> Removed {
         let value = match key.try_as_name(super::as_name::Seal) {
             Ok(Cow::Borrowed(name)) => self.inner.remove(name),
@@ -571,7 +602,7 @@ impl<'a> IntoIterator for &'a HeaderMap {
     }
 }
 
-/// Iterator for all values with the same header name.
+/// Iterator for references of [`HeaderValue`]s with the same associated [`HeaderName`].
 ///
 /// See [`HeaderMap::get_all`].
 #[derive(Debug)]
@@ -613,18 +644,32 @@ impl<'a> Iterator for GetAll<'a> {
     }
 }
 
-/// Iterator for owned [`HeaderValue`]s with the same associated [`HeaderName`] returned from methods
+/// Iterator for owned [`HeaderValue`]s with the same associated [`HeaderName`] returned from
-/// on [`HeaderMap`] that remove or replace items.
+/// methods that remove or replace items.
+///
+/// See [`HeaderMap::insert`] and [`HeaderMap::remove`].
 #[derive(Debug)]
 pub struct Removed {
     inner: Option<smallvec::IntoIter<[HeaderValue; 4]>>,
 }
 
-impl<'a> Removed {
+impl Removed {
     fn new(value: Option<Value>) -> Self {
         let inner = value.map(|value| value.inner.into_iter());
         Self { inner }
     }
+
+    /// Returns true if iterator contains no elements, without consuming it.
+    ///
+    /// If called immediately after [`HeaderMap::insert`] or [`HeaderMap::remove`], it will indicate
+    /// wether any items were actually replaced or removed, respectively.
+    pub fn is_empty(&self) -> bool {
+        match self.inner {
+            // size hint lower bound of smallvec is the correct length
+            Some(ref iter) => iter.size_hint().0 == 0,
+            None => true,
+        }
+    }
 }
 
 impl Iterator for Removed {
@@ -945,6 +990,53 @@ mod tests {
         assert_eq!(vals.next(), removed.next().as_ref());
     }
 
+    #[test]
+    fn get_all_iteration_order_matches_insertion_order() {
+        let mut map = HeaderMap::new();
+
+        let mut vals = map.get_all(header::COOKIE);
+        assert!(vals.next().is_none());
+
+        map.append(header::COOKIE, HeaderValue::from_static("1"));
+        let mut vals = map.get_all(header::COOKIE);
+        assert_eq!(vals.next().unwrap().as_bytes(), b"1");
+        assert!(vals.next().is_none());
+
+        map.append(header::COOKIE, HeaderValue::from_static("2"));
+        let mut vals = map.get_all(header::COOKIE);
+        assert_eq!(vals.next().unwrap().as_bytes(), b"1");
+        assert_eq!(vals.next().unwrap().as_bytes(), b"2");
+        assert!(vals.next().is_none());
+
+        map.append(header::COOKIE, HeaderValue::from_static("3"));
+        map.append(header::COOKIE, HeaderValue::from_static("4"));
+        map.append(header::COOKIE, HeaderValue::from_static("5"));
+        let mut vals = map.get_all(header::COOKIE);
+        assert_eq!(vals.next().unwrap().as_bytes(), b"1");
+        assert_eq!(vals.next().unwrap().as_bytes(), b"2");
+        assert_eq!(vals.next().unwrap().as_bytes(), b"3");
+        assert_eq!(vals.next().unwrap().as_bytes(), b"4");
+        assert_eq!(vals.next().unwrap().as_bytes(), b"5");
+        assert!(vals.next().is_none());
+
+        let _ = map.insert(header::COOKIE, HeaderValue::from_static("6"));
+        let mut vals = map.get_all(header::COOKIE);
+        assert_eq!(vals.next().unwrap().as_bytes(), b"6");
+        assert!(vals.next().is_none());
+
+        let _ = map.insert(header::COOKIE, HeaderValue::from_static("7"));
+        let _ = map.insert(header::COOKIE, HeaderValue::from_static("8"));
+        let mut vals = map.get_all(header::COOKIE);
+        assert_eq!(vals.next().unwrap().as_bytes(), b"8");
+        assert!(vals.next().is_none());
+
+        map.append(header::COOKIE, HeaderValue::from_static("9"));
+        let mut vals = map.get_all(header::COOKIE);
+        assert_eq!(vals.next().unwrap().as_bytes(), b"8");
+        assert_eq!(vals.next().unwrap().as_bytes(), b"9");
+        assert!(vals.next().is_none());
+    }
+
     fn owned_pair<'a>(
         (name, val): (&'a HeaderName, &'a HeaderValue),
     ) -> (HeaderName, HeaderValue) {

actix-http/src/header/mod.rs

@@ -29,16 +29,14 @@ pub use http::header::{
     X_FRAME_OPTIONS, X_XSS_PROTECTION,
 };
 
-use crate::error::ParseError;
+use crate::{error::ParseError, HttpMessage};
-use crate::HttpMessage;
 
 mod as_name;
 mod into_pair;
 mod into_value;
-mod utils;
-
 pub(crate) mod map;
 mod shared;
+mod utils;
 
 #[doc(hidden)]
 pub use self::shared::*;
@@ -46,10 +44,10 @@ pub use self::shared::*;
 pub use self::as_name::AsHeaderName;
 pub use self::into_pair::IntoHeaderPair;
 pub use self::into_value::IntoHeaderValue;
-#[doc(hidden)]
+pub use self::map::{GetAll, HeaderMap, Removed};
-pub use self::map::GetAll;
+pub use self::utils::{
-pub use self::map::HeaderMap;
+    fmt_comma_delimited, from_comma_delimited, from_one_raw_str, http_percent_encode,
-pub use self::utils::*;
+};
 
 /// A trait for any object that already represents a valid header field and value.
 pub trait Header: IntoHeaderValue {
@@ -68,7 +66,7 @@ impl From<http::HeaderMap> for HeaderMap {
 }
 
 /// This encode set is used for HTTP header values and is defined at
-/// https://tools.ietf.org/html/rfc5987#section-3.2.
+/// <https://tools.ietf.org/html/rfc5987#section-3.2>.
 pub(crate) const HTTP_VALUE: &AsciiSet = &CONTROLS
     .add(b' ')
     .add(b'"')

actix-http/src/header/utils.rs

@@ -56,6 +56,7 @@ where
 
 /// Percent encode a sequence of bytes with a character set defined in
 /// <https://tools.ietf.org/html/rfc5987#section-3.2>
+#[inline]
 pub fn http_percent_encode(f: &mut fmt::Formatter<'_>, bytes: &[u8]) -> fmt::Result {
     let encoded = percent_encoding::percent_encode(bytes, HTTP_VALUE);
     fmt::Display::fmt(&encoded, f)

actix-http/src/ws/proto.rs

@@ -220,7 +220,7 @@ impl<T: Into<String>> From<(CloseCode, T)> for CloseReason {
     }
 }
 
-/// The WebSocket GUID as stated in the spec. See https://tools.ietf.org/html/rfc6455#section-1.3.
+/// The WebSocket GUID as stated in the spec. See <https://tools.ietf.org/html/rfc6455#section-1.3>.
 static WS_GUID: &[u8] = b"258EAFA5-E914-47DA-95CA-C5AB0DC85B11";
 
 /// Hashes the `Sec-WebSocket-Key` header according to the WebSocket spec.

src/extract.rs

@@ -10,6 +10,7 @@ use std::{
 use actix_http::http::{Method, Uri};
 use actix_utils::future::{ok, Ready};
 use futures_core::ready;
+use pin_project_lite::pin_project;
 
 use crate::{dev::Payload, Error, HttpRequest};
 
@@ -139,10 +140,11 @@ where
     }
 }
 
-#[pin_project::pin_project]
+pin_project! {
-pub struct FromRequestOptFuture<Fut> {
+    pub struct FromRequestOptFuture<Fut> {
-    #[pin]
+        #[pin]
-    fut: Fut,
+        fut: Fut,
+    }
 }
 
 impl<Fut, T, E> Future for FromRequestOptFuture<Fut>
@@ -226,10 +228,11 @@ where
     }
 }
 
-#[pin_project::pin_project]
+pin_project! {
-pub struct FromRequestResFuture<Fut> {
+    pub struct FromRequestResFuture<Fut> {
-    #[pin]
+        #[pin]
-    fut: Fut,
+        fut: Fut,
+    }
 }
 
 impl<Fut, T, E> Future for FromRequestResFuture<Fut>
@@ -297,102 +300,104 @@ impl FromRequest for () {
     }
 }
 
-macro_rules! tuple_from_req ({$fut_type:ident, $(($n:tt, $T:ident)),+} => {
+#[doc(hidden)]
-
+#[allow(non_snake_case)]
-    // This module is a trick to get around the inability of
+mod tuple_from_req {
-    // `macro_rules!` macros to make new idents. We want to make
-    // a new `FutWrapper` struct for each distinct invocation of
-    // this macro. Ideally, we would name it something like
-    // `FutWrapper_$fut_type`, but this can't be done in a macro_rules
-    // macro.
-    //
-    // Instead, we put everything in a module named `$fut_type`, thus allowing
-    // us to use the name `FutWrapper` without worrying about conflicts.
-    // This macro only exists to generate trait impls for tuples - these
-    // are inherently global, so users don't have to care about this
-    // weird trick.
-    #[allow(non_snake_case)]
-    mod $fut_type {
-
-        // Bring everything into scope, so we don't need
-        // redundant imports
-        use super::*;
-
-        /// A helper struct to allow us to pin-project through
-        /// to individual fields
-        #[pin_project::pin_project]
-        struct FutWrapper<$($T: FromRequest),+>($(#[pin] $T::Future),+);
-
-        /// FromRequest implementation for tuple
-        #[doc(hidden)]
-        #[allow(unused_parens)]
-        impl<$($T: FromRequest + 'static),+> FromRequest for ($($T,)+)
-        {
-            type Error = Error;
-            type Future = $fut_type<$($T),+>;
-
-            fn from_request(req: &HttpRequest, payload: &mut Payload) -> Self::Future {
-                $fut_type {
-                    items: <($(Option<$T>,)+)>::default(),
-                    futs: FutWrapper($($T::from_request(req, payload),)+),
-                }
-            }
-        }
-
-        #[doc(hidden)]
-        #[pin_project::pin_project]
-        pub struct $fut_type<$($T: FromRequest),+> {
-            items: ($(Option<$T>,)+),
-            #[pin]
-            futs: FutWrapper<$($T,)+>,
-        }
-
-        impl<$($T: FromRequest),+> Future for $fut_type<$($T),+>
-        {
-            type Output = Result<($($T,)+), Error>;
-
-            fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
-                let mut this = self.project();
-
-                let mut ready = true;
-                $(
-                    if this.items.$n.is_none() {
-                        match this.futs.as_mut().project().$n.poll(cx) {
-                            Poll::Ready(Ok(item)) => {
-                                this.items.$n = Some(item);
-                            }
-                            Poll::Pending => ready = false,
-                            Poll::Ready(Err(e)) => return Poll::Ready(Err(e.into())),
-                        }
-                    }
-                )+
-
-                if ready {
-                    Poll::Ready(Ok(
-                        ($(this.items.$n.take().unwrap(),)+)
-                    ))
-                } else {
-                    Poll::Pending
-                }
-            }
-        }
-    }
-});
-
-#[rustfmt::skip]
-mod m {
     use super::*;
 
-    tuple_from_req!(TupleFromRequest1, (0, A));
+    macro_rules! tuple_from_req {
-    tuple_from_req!(TupleFromRequest2, (0, A), (1, B));
+        ($fut: ident; $($T: ident),*) => {
-    tuple_from_req!(TupleFromRequest3, (0, A), (1, B), (2, C));
+            /// FromRequest implementation for tuple
-    tuple_from_req!(TupleFromRequest4, (0, A), (1, B), (2, C), (3, D));
+            #[allow(unused_parens)]
-    tuple_from_req!(TupleFromRequest5, (0, A), (1, B), (2, C), (3, D), (4, E));
+            impl<$($T: FromRequest + 'static),+> FromRequest for ($($T,)+)
-    tuple_from_req!(TupleFromRequest6, (0, A), (1, B), (2, C), (3, D), (4, E), (5, F));
+            {
-    tuple_from_req!(TupleFromRequest7, (0, A), (1, B), (2, C), (3, D), (4, E), (5, F), (6, G));
+                type Error = Error;
-    tuple_from_req!(TupleFromRequest8, (0, A), (1, B), (2, C), (3, D), (4, E), (5, F), (6, G), (7, H));
+                type Future = $fut<$($T),+>;
-    tuple_from_req!(TupleFromRequest9, (0, A), (1, B), (2, C), (3, D), (4, E), (5, F), (6, G), (7, H), (8, I));
+
-    tuple_from_req!(TupleFromRequest10, (0, A), (1, B), (2, C), (3, D), (4, E), (5, F), (6, G), (7, H), (8, I), (9, J));
+                fn from_request(req: &HttpRequest, payload: &mut Payload) -> Self::Future {
+                    $fut {
+                        $(
+                            $T: ExtractFuture::Future {
+                                fut: $T::from_request(req, payload)
+                            },
+                        )+
+                    }
+                }
+            }
+
+            pin_project! {
+                pub struct $fut<$($T: FromRequest),+> {
+                    $(
+                        #[pin]
+                        $T: ExtractFuture<$T::Future, $T>,
+                    )+
+                }
+            }
+
+            impl<$($T: FromRequest),+> Future for $fut<$($T),+>
+            {
+                type Output = Result<($($T,)+), Error>;
+
+                fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
+                    let mut this = self.project();
+
+                    let mut ready = true;
+                    $(
+                        match this.$T.as_mut().project() {
+                            ExtractProj::Future { fut } => match fut.poll(cx) {
+                                Poll::Ready(Ok(output)) => {
+                                    let _ = this.$T.as_mut().project_replace(ExtractFuture::Done { output });
+                                },
+                                Poll::Ready(Err(e)) => return Poll::Ready(Err(e.into())),
+                                Poll::Pending => ready = false,
+                            },
+                            ExtractProj::Done { .. } => {},
+                            ExtractProj::Empty => unreachable!("FromRequest polled after finished"),
+                        }
+                    )+
+
+                    if ready {
+                        Poll::Ready(Ok(
+                            ($(
+                                match this.$T.project_replace(ExtractFuture::Empty) {
+                                    ExtractReplaceProj::Done { output } => output,
+                                    _ => unreachable!("FromRequest polled after finished"),
+                                },
+                            )+)
+                        ))
+                    } else {
+                        Poll::Pending
+                    }
+                }
+            }
+        };
+    }
+
+    pin_project! {
+        #[project = ExtractProj]
+        #[project_replace = ExtractReplaceProj]
+        enum ExtractFuture<Fut, Res> {
+            Future {
+                #[pin]
+                fut: Fut
+            },
+            Done {
+                output: Res,
+            },
+            Empty
+        }
+    }
+
+    tuple_from_req! { TupleFromRequest1; A }
+    tuple_from_req! { TupleFromRequest2; A, B }
+    tuple_from_req! { TupleFromRequest3; A, B, C }
+    tuple_from_req! { TupleFromRequest4; A, B, C, D }
+    tuple_from_req! { TupleFromRequest5; A, B, C, D, E }
+    tuple_from_req! { TupleFromRequest6; A, B, C, D, E, F }
+    tuple_from_req! { TupleFromRequest7; A, B, C, D, E, F, G }
+    tuple_from_req! { TupleFromRequest8; A, B, C, D, E, F, G, H }
+    tuple_from_req! { TupleFromRequest9; A, B, C, D, E, F, G, H, I }
+    tuple_from_req! { TupleFromRequest10; A, B, C, D, E, F, G, H, I, J }
 }
 
 #[cfg(test)]
@@ -494,4 +499,26 @@ mod tests {
         let method = Method::extract(&req).await.unwrap();
         assert_eq!(method, Method::GET);
     }
+
+    #[actix_rt::test]
+    async fn test_concurrent() {
+        let (req, mut pl) = TestRequest::default()
+            .uri("/foo/bar")
+            .method(Method::GET)
+            .insert_header((header::CONTENT_TYPE, "application/x-www-form-urlencoded"))
+            .insert_header((header::CONTENT_LENGTH, "11"))
+            .set_payload(Bytes::from_static(b"hello=world"))
+            .to_http_parts();
+        let (method, uri, form) = <(Method, Uri, Form<Info>)>::from_request(&req, &mut pl)
+            .await
+            .unwrap();
+        assert_eq!(method, Method::GET);
+        assert_eq!(uri.path(), "/foo/bar");
+        assert_eq!(
+            form,
+            Form(Info {
+                hello: "world".into()
+            })
+        );
+    }
 }