actix / actix-web

Actix Web is a powerful, pragmatic, and extremely fast web framework for Rust.
https://actix.rs
Apache License 2.0
21.74k stars 1.68k forks source link

keepalives not working #631

Closed jeffsmith82 closed 5 years ago

jeffsmith82 commented 5 years ago

I'm testing keepalive requests with the following code but when using Apache bench but I don't get keepalives.

extern crate actix_web;
use actix_web::{server, App, HttpRequest};

fn hello_world(_req: &HttpRequest) -> &'static str {
    "Hello World!"
}

fn main() {
    server::new(|| App::new().resource("/", |r| r.f(hello_world)))
        .keep_alive(server::KeepAlive::Tcp(75))
        .bind("0.0.0.0:8080")
        .expect("Can not bind to port 8080")
        .run();
}
ab -k -n 300000 -c 10 http://10.1.11.206:8080/
...
Concurrency Level:      10
Time taken for tests:   17.192 seconds
Complete requests:      62671
Failed requests:        0
Write errors:           0
Keep-Alive requests:    0
Total transferred:      8084817 bytes
HTML transferred:       752076 bytes
Requests per second:    3645.36 [#/sec] (mean)
Time per request:       2.743 [ms] (mean)
Time per request:       0.274 [ms] (mean, across all concurrent requests)
Transfer rate:          459.24 [Kbytes/sec] received

This is showing no requests where Keep-Alive and the performance is pretty bad. This is on Centos 7.6, rustc 1.31.0 and setting actix-web = "0.7". Am I missing something in the docs that i need to set ?

DoumanAsh commented 5 years ago

I'm not sure what you want to achieve exactly...

Tcp variant is Use SO_KEEPALIVE socket option, value in seconds

You might want to use KeepAlive::Timeout so that actix-web would control it?

jeffsmith82 commented 5 years ago

@DoumanAsh I'm trying to get http keepalive working so I can reuse connections.

I tried .keep_alive(server::KeepAlive::Timeout(75)) which is what i think you are suggesting and still doesn't work.

also read the docs here https://actix.rs/docs/server/ and tried .keep_alive(75) but doesn't work.

DoumanAsh commented 5 years ago

It would be great if somehow you could elaborate what it means by not working(I'm not familiar with that tool of yours)...

cc @fafhrd91 if you can give some input?

jeffsmith82 commented 5 years ago

@DoumanAsh apache bench is the tool https://httpd.apache.org/docs/2.4/programs/ab.html

basically with keepalive working ab should connect once per number of connections I want in this case 10 "-c 10" and then reuse those connections for subsequent requests. This means no overhead for creating a connection for every request as each one will serve multiple http requests.

Using a simple go webserver where keepalive works and turning it off shows the performance difference.

--With keepalive working Keep-Alive requests: 300000 Requests per second: 28320.61 [#/sec] (mean)

--no keepalive Requests per second: 4224.12 [#/sec] (mean)

DoumanAsh commented 5 years ago

@jeffsmith82 Can you please check that with your settings you have header Connection: keep-alive in response? If you use HTTP2, then there should be no such header and it is default behavior

jeffsmith82 commented 5 years ago

Ran this and it doesn't appear to be sending back any Connection header.

ab -n 10 -c 2 -v 3 -k http://127.0.0.2:8080/

LOG: Response code = 200
LOG: header received:
HTTP/1.0 200 OK
content-length: 12
content-type: text/plain; charset=utf-8
date: Sun, 16 Dec 2018 10:13:08 GMT

Hello World!

Wireshark is showing the client sending "Connection: Keep-Alive"

GET / HTTP/1.0
Connection: Keep-Alive
Host: 127.0.0.2:8080
User-Agent: ApacheBench/2.3
Accept: */*

HTTP/1.0 200 OK
content-length: 12
content-type: text/plain; charset=utf-8
date: Sun, 16 Dec 2018 10:19:50 GMT

Hello World!
DoumanAsh commented 5 years ago

@jeffsmith82 ~Keep-Alive is available only in HTTP 1.1, 1.0 cannot be used with keep-alive so we only send this header in 1.1~

Actually I may be mistaken, I need to re-check kit

It seems we should set keep-alive only in 1.0 case because 1.1. makes it default

DoumanAsh commented 5 years ago

@jeffsmith82 Thanks for help, I noticed that we didn't perform case-insensitive comparison on client headers when parsing HTTP1 messages

DoumanAsh commented 5 years ago

@jeffsmith82 If possible could you please test my branch? Since that tool uses only HTTP 1.0 it is likely always send Capital Keep-Alive https://github.com/DoumanAsh/actix-web/tree/Http1_ignore_header_case

jeffsmith82 commented 5 years ago

@DoumanAsh Are there any docs on how to test your branch ?

I'm assuming git clone it and then cargo build but not sure how to tell my toy version how to run that version instead of the 0.7 version from crates.io.

DoumanAsh commented 5 years ago

Read on how to patch crates.io dependencies https://doc.rust-lang.org/cargo/reference/manifest.html#the-patch-section

There is also simple example

jeffsmith82 commented 5 years ago

@DoumanAsh sorry don't understand what i'm doing wrong here trying to use this as Cargo.toml but get errors, can you point out what i'm doing wrong.

[package]
name = "actix_hello"
version = "0.1.0"
authors = ["jeffreys"]

[dependencies]

[patch.'https://github.com/DoumanAsh/actix-web']
actix-web = {git = 'https://github.com/DoumanAsh/actix-web', branch = 'Http1_ignore_header_case' }
DoumanAsh commented 5 years ago

You should do [patch.crates-io]

But actually you can specify my repo directly in dependencies:

[dependencies]
actix-web = {git = 'https://github.com/DoumanAsh/actix-web', branch = 'Http1_ignore_header_case' }
jeffsmith82 commented 5 years ago

@DoumanAsh It seems to work, it's using keepalive connections now and request per second has jumped from 3600 ish per second to 108,377 per second which is nice. Cheers for fixing this it was driving me mad trying to work out why the performance was so bad.

Concurrency Level:      10
Time taken for tests:   2.768 seconds
Complete requests:      300000
Failed requests:        0
Keep-Alive requests:    300000
Total transferred:      45900000 bytes
HTML transferred:       3600000 bytes
Requests per second:    108377.47 [#/sec] (mean)
Time per request:       0.092 [ms] (mean)
Time per request:       0.009 [ms] (mean, across all concurrent requests)
Transfer rate:          16193.12 [Kbytes/sec] received
DoumanAsh commented 5 years ago

Great, thx, it should be only issue for HTTP 1.0, on HTTP 1.1 you would still get keep-alive benefit and modern clients use lower case headers

nocduro commented 5 years ago

Hi, I'm also having some issues with keepalives not working. I'm using the same code as the first comment and when loading the page in a web browser I don't see the Connection header in the response.

Code:

use actix_web::{server, App, HttpRequest};

fn hello_world(_req: &HttpRequest) -> &'static str {
    "Hello World!"
}

fn main() {
    server::new(|| App::new().resource("/", |r| r.f(hello_world)))
        .keep_alive(75)
        .bind("0.0.0.0:8080")
        .expect("Can not bind to port 8080")
        .run();
}

OS: Windows 10 (I tested running a server on WSL and same thing) Browser: Firefox and Chrome give same results actix-web: v0.7.18 actix-net: v0.2.6 actix: v0.7.9

Request headers:

Host: localhost:8080
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Connection: keep-alive
Upgrade-Insecure-Requests: 1
DNT: 1

Response headers:

HTTP/1.1 200 OK
content-length: 12
content-type: text/plain; charset=utf-8
date: Mon, 11 Mar 2019 22:14:22 GMT

I know I'm posting this comment close to the 1.0 release, so if it's fixed in 1.0 I will wait. Thanks!

DoumanAsh commented 5 years ago

That's very weird, on HTTP1 you should have keep alive even without Connection: keep-alive header

I'll need to check how we add keep alive realted headers to responses

DoumanAsh commented 5 years ago

@nocduro Now I remember, on HTTP/1.1 it is not necessary to send out Connection: keep-alive since it is default behavior and we send it only for HTTP/1.0 so there should be no problem, keep alive will work unless your client would mis behave(we send only Connection: close if keep alive is disabled)

nocduro commented 5 years ago

Ahh that makes sense. Thanks for the reply!

PragmaTwice commented 3 years ago

Why Tcp(usize) in KeepAlive enum is removed now? I need it. 😢

Aurillium commented 6 months ago

I'm not sure the behaviour for keep-alive is the same for all systems, it seems to be all good on my laptop (using curl), but on my iPhone (the curse of all things standard) connections seem to close when I close the app if they're not marked with Connection: keep-alive, and I haven't found a way to add that header back no matter what I try (even the default headers middleware, I don't want to make my own right now because it's 2am though)

As long as the connection is opened and closed while I'm on the app I get all the data, but if I leave the app at any time, everything gets discarded which is a bit painful considering it's a stream that's supposed to give me notifications

Is there an easy way to add back the Connection: keep-alive header? If not I think it's probably a good idea to at least allow it to exist for weird edge cases like this

robjtede commented 5 months ago

This issue was closed 6 years ago. Please consider opening a new issue instead of leaving a comment here.