Merge pull request #3 from actix/master

0125

.github/ISSUE_TEMPLATE/bug_report.md

@@ -1,3 +1,8 @@
+---
+name: bug report
+about: create a bug report
+---
+
 Your issue may already be reported!
 Please search on the [Actix Web issue tracker](https://github.com/actix/actix-web/issues) before creating one.
 
@@ -29,4 +34,4 @@ Please search on the [Actix Web issue tracker](https://github.com/actix/actix-we
 <!--- Include as many relevant details about the environment you experienced the bug in -->
 
 * Rust Version (I.e, output of `rustc -V`):
-* Actix Web Version: 
+* Actix Web Version:

.github/workflows/bench.yml

@@ -24,19 +24,24 @@ jobs:
         uses: actions/cache@v1
         with:
           path: ~/.cargo/registry
-          key: ${{ matrix.version }}-x86_64-unknown-linux-gnu-registry-${{ hashFiles('**/Cargo.lock') }}
+          key: ${{ matrix.version }}-x86_64-unknown-linux-gnu-registry-trimmed-${{ hashFiles('**/Cargo.lock') }}
       - name: Cache cargo index
         uses: actions/cache@v1
         with:
           path: ~/.cargo/git
-          key: ${{ matrix.version }}-x86_64-unknown-linux-gnu-cargo-index-${{ hashFiles('**/Cargo.lock') }}
+          key: ${{ matrix.version }}-x86_64-unknown-linux-gnu-cargo-index-trimmed-${{ hashFiles('**/Cargo.lock') }}
       - name: Cache cargo build
         uses: actions/cache@v1
         with:
           path: target
-          key: ${{ matrix.version }}-x86_64-unknown-linux-gnu-cargo-build-${{ hashFiles('**/Cargo.lock') }}
+          key: ${{ matrix.version }}-x86_64-unknown-linux-gnu-cargo-build-trimmed-${{ hashFiles('**/Cargo.lock') }}
 
       - name: Check benchmark
         uses: actions-rs/cargo@v1
         with:
           command: bench
+
+      - name: Clear the cargo caches
+        run: |
+          cargo install cargo-cache --no-default-features --features ci-autoclean
+          cargo-cache

.github/workflows/macos.yml

@@ -32,17 +32,17 @@ jobs:
         uses: actions/cache@v1
         with:
           path: ~/.cargo/registry
-          key: ${{ matrix.version }}-x86_64-apple-darwin-cargo-registry-${{ hashFiles('**/Cargo.lock') }}
+          key: ${{ matrix.version }}-x86_64-apple-darwin-cargo-registry-trimmed-${{ hashFiles('**/Cargo.lock') }}
       - name: Cache cargo index
         uses: actions/cache@v1
         with:
           path: ~/.cargo/git
-          key: ${{ matrix.version }}-x86_64-apple-darwin-cargo-index-${{ hashFiles('**/Cargo.lock') }}
+          key: ${{ matrix.version }}-x86_64-apple-darwin-cargo-index-trimmed-${{ hashFiles('**/Cargo.lock') }}
       - name: Cache cargo build
         uses: actions/cache@v1
         with:
           path: target
-          key: ${{ matrix.version }}-x86_64-apple-darwin-cargo-build-${{ hashFiles('**/Cargo.lock') }}
+          key: ${{ matrix.version }}-x86_64-apple-darwin-cargo-build-trimmed-${{ hashFiles('**/Cargo.lock') }}
 
       - name: check build
         uses: actions-rs/cargo@v1
@@ -57,3 +57,8 @@ jobs:
           args: --all --all-features --no-fail-fast -- --nocapture
                 --skip=test_h2_content_length
                 --skip=test_reading_deflate_encoding_large_random_rustls
+
+      - name: Clear the cargo caches
+        run: |
+          cargo install cargo-cache --no-default-features --features ci-autoclean
+          cargo-cache

.github/workflows/windows.yml

@@ -35,17 +35,17 @@ jobs:
         uses: actions/cache@v1
         with:
           path: ~/.cargo/registry
-          key: ${{ matrix.version }}-x86_64-pc-windows-msvc-cargo-registry-${{ hashFiles('**/Cargo.lock') }}
+          key: ${{ matrix.version }}-x86_64-pc-windows-msvc-cargo-registry-trimmed-${{ hashFiles('**/Cargo.lock') }}
       - name: Cache cargo index
         uses: actions/cache@v1
         with:
           path: ~/.cargo/git
-          key: ${{ matrix.version }}-x86_64-pc-windows-msvc-cargo-index-${{ hashFiles('**/Cargo.lock') }}
+          key: ${{ matrix.version }}-x86_64-pc-windows-msvc-cargo-index-trimmed-${{ hashFiles('**/Cargo.lock') }}
       - name: Cache cargo build
         uses: actions/cache@v1
         with:
           path: target
-          key: ${{ matrix.version }}-x86_64-pc-windows-msvc-cargo-build-${{ hashFiles('**/Cargo.lock') }}
+          key: ${{ matrix.version }}-x86_64-pc-windows-msvc-cargo-build-trimmed-${{ hashFiles('**/Cargo.lock') }}
 
       - name: Install OpenSSL
         run: |
@@ -74,3 +74,8 @@ jobs:
                 --skip=test_expect_continue
                 --skip=test_http10_keepalive
                 --skip=test_slow_request
+
+      - name: Clear the cargo caches
+        run: |
+          cargo install cargo-cache --no-default-features --features ci-autoclean
+          cargo-cache

Cargo.toml

@@ -99,6 +99,7 @@ env_logger = "0.6"
 serde_derive = "1.0"
 brotli2 = "0.3.2"
 flate2 = "1.0.13"
+criterion = "0.3"
 
 [profile.release]
 lto = true
@@ -116,3 +117,11 @@ actix-session = { path = "actix-session" }
 actix-files = { path = "actix-files" }
 actix-multipart = { path = "actix-multipart" }
 awc = { path = "awc" }
+
+[[bench]]
+name = "server"
+harness = false
+
+[[bench]]
+name = "service"
+harness = false

actix-http/src/cloneable.rs

@@ -1,4 +1,4 @@
-use std::cell::UnsafeCell;
+use std::cell::RefCell;
 use std::rc::Rc;
 use std::task::{Context, Poll};
 
@@ -6,11 +6,15 @@ use actix_service::Service;
 
 #[doc(hidden)]
 /// Service that allows to turn non-clone service to a service with `Clone` impl
-pub(crate) struct CloneableService<T: Service>(Rc<UnsafeCell<T>>);
+///
+/// # Panics
+/// CloneableService might panic with some creative use of thread local storage.
+/// See https://github.com/actix/actix-web/issues/1295 for example
+pub(crate) struct CloneableService<T: Service>(Rc<RefCell<T>>);
 
 impl<T: Service> CloneableService<T> {
     pub(crate) fn new(service: T) -> Self {
-        Self(Rc::new(UnsafeCell::new(service)))
+        Self(Rc::new(RefCell::new(service)))
     }
 }
 
@@ -27,10 +31,10 @@ impl<T: Service> Service for CloneableService<T> {
     type Future = T::Future;
 
     fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
-        unsafe { &mut *self.0.as_ref().get() }.poll_ready(cx)
+        self.0.borrow_mut().poll_ready(cx)
     }
 
     fn call(&mut self, req: T::Request) -> Self::Future {
-        unsafe { &mut *self.0.as_ref().get() }.call(req)
+        self.0.borrow_mut().call(req)
     }
 }

actix-http/src/cookie/draft.rs

@@ -14,13 +14,13 @@ use std::fmt;
 /// normal. In some browsers, this will implicitly handle the cookie as if "Lax"
 /// and in others, "None". It's best to explicitly set the `SameSite` attribute
 /// to avoid inconsistent behavior.
-/// 
+///
 /// **Note:** Depending on browser, the `Secure` attribute may be required for
 /// `SameSite` "None" cookies to be accepted.
 ///
 /// **Note:** This cookie attribute is an HTTP draft! Its meaning and definition
 /// are subject to change.
-/// 
+///
 /// More info about these draft changes can be found in the draft spec:
 /// - https://tools.ietf.org/html/draft-west-cookie-incrementalism-00
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]

benches/server.rs

@@ -0,0 +1,64 @@
+use actix_web::{test, web, App, HttpResponse};
+use awc::Client;
+use criterion::{criterion_group, criterion_main, Criterion};
+use futures::future::join_all;
+
+const STR: &str = "Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World \
+                   Hello World Hello World Hello World Hello World Hello World";
+
+// benchmark sending all requests at the same time
+fn bench_async_burst(c: &mut Criterion) {
+    let srv = test::start(|| {
+        App::new()
+            .service(web::resource("/").route(web::to(|| HttpResponse::Ok().body(STR))))
+    });
+
+    // We are using System here, since Runtime requires preinitialized tokio
+    // Maybe add to actix_rt docs
+    let url = srv.url("/");
+    let mut rt = actix_rt::System::new("test");
+
+    c.bench_function("get_body_async_burst", move |b| {
+        b.iter_custom(|iters| {
+            let client = Client::new().get(url.clone()).freeze().unwrap();
+
+            let start = std::time::Instant::now();
+            // benchmark body
+            let resps = rt.block_on(async move {
+                let burst = (0..iters).map(|_| client.send());
+                join_all(burst).await
+            });
+            let elapsed = start.elapsed();
+
+            // if there are failed requests that might be an issue
+            let failed = resps.iter().filter(|r| r.is_err()).count();
+            if failed > 0 {
+                eprintln!("failed {} requests (might be bench timeout)", failed);
+            };
+
+            elapsed
+        })
+    });
+}
+
+criterion_group!(server_benches, bench_async_burst);
+criterion_main!(server_benches);

benches/service.rs

@@ -0,0 +1,108 @@
+use actix_service::Service;
+use actix_web::dev::{ServiceRequest, ServiceResponse};
+use actix_web::{web, App, Error, HttpResponse};
+use criterion::{criterion_main, Criterion};
+use std::cell::RefCell;
+use std::rc::Rc;
+
+use actix_web::test::{init_service, ok_service, TestRequest};
+
+/// Criterion Benchmark for async Service
+/// Should be used from within criterion group:
+/// ```rust,ignore
+/// let mut criterion: ::criterion::Criterion<_> =
+///     ::criterion::Criterion::default().configure_from_args();
+/// bench_async_service(&mut criterion, ok_service(), "async_service_direct");
+/// ```
+///
+/// Usable for benching Service wrappers:
+/// Using minimum service code implementation we first measure
+/// time to run minimum service, then measure time with wrapper.
+///
+/// Sample output
+/// async_service_direct    time:   [1.0908 us 1.1656 us 1.2613 us]
+pub fn bench_async_service<S>(c: &mut Criterion, srv: S, name: &str)
+where
+    S: Service<Request = ServiceRequest, Response = ServiceResponse, Error = Error>
+        + 'static,
+{
+    let mut rt = actix_rt::System::new("test");
+    let srv = Rc::new(RefCell::new(srv));
+
+    let req = TestRequest::default().to_srv_request();
+    assert!(rt
+        .block_on(srv.borrow_mut().call(req))
+        .unwrap()
+        .status()
+        .is_success());
+
+    // start benchmark loops
+    c.bench_function(name, move |b| {
+        b.iter_custom(|iters| {
+            let srv = srv.clone();
+            // exclude request generation, it appears it takes significant time vs call (3us vs 1us)
+            let reqs: Vec<_> = (0..iters)
+                .map(|_| TestRequest::default().to_srv_request())
+                .collect();
+            let start = std::time::Instant::now();
+            // benchmark body
+            rt.block_on(async move {
+                for req in reqs {
+                    srv.borrow_mut().call(req).await.unwrap();
+                }
+            });
+            let elapsed = start.elapsed();
+            // check that at least first request succeeded
+            elapsed
+        })
+    });
+}
+
+async fn index(req: ServiceRequest) -> Result<ServiceResponse, Error> {
+    Ok(req.into_response(HttpResponse::Ok().finish()))
+}
+
+// Benchmark basic WebService directly
+// this approach is usable for benching WebService, though it adds some time to direct service call:
+// Sample results on MacBook Pro '14
+// time:   [2.0724 us 2.1345 us 2.2074 us]
+fn async_web_service(c: &mut Criterion) {
+    let mut rt = actix_rt::System::new("test");
+    let srv = Rc::new(RefCell::new(rt.block_on(init_service(
+        App::new().service(web::service("/").finish(index)),
+    ))));
+
+    let req = TestRequest::get().uri("/").to_request();
+    assert!(rt
+        .block_on(srv.borrow_mut().call(req))
+        .unwrap()
+        .status()
+        .is_success());
+
+    // start benchmark loops
+    c.bench_function("async_web_service_direct", move |b| {
+        b.iter_custom(|iters| {
+            let srv = srv.clone();
+            let reqs = (0..iters).map(|_| TestRequest::get().uri("/").to_request());
+
+            let start = std::time::Instant::now();
+            // benchmark body
+            rt.block_on(async move {
+                for req in reqs {
+                    srv.borrow_mut().call(req).await.unwrap();
+                }
+            });
+            let elapsed = start.elapsed();
+            // check that at least first request succeeded
+            elapsed
+        })
+    });
+}
+
+pub fn service_benches() {
+    let mut criterion: ::criterion::Criterion<_> =
+        ::criterion::Criterion::default().configure_from_args();
+    bench_async_service(&mut criterion, ok_service(), "async_service_direct");
+    async_web_service(&mut criterion);
+}
+criterion_main!(service_benches);