Actix-web: HTTP request payload does not handle backpressure

[bancek]

Request payload stream should handle back pressure. If server cannot process the request body fast enough (or does not process it at all) the client should slow down. This is also a DoS vector.

It looks like the whole decoding architecture is written in a way that does not allow backpressure:

https://github.com/actix/actix-web/blob/748289f/actix-http/src/encoding/decoder.rs#L98

I've created a repository to reproduce that: https://github.com/bancek/actix-request-body-backpressure

$ cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.07s
     Running `target/debug/actix-request-body-backpressure`
Listening on 127.0.0.1:8080

...
$ truncate -s 50M 50M.bin
$ curl -v http://127.0.0.1:8080/ -X PUT --data-binary @50M.bin > /dev/null
...

Upload chunk 1900544
Upload chunk 24215552
Upload chunk 26312704

The same can be reproduced by just using a payload: web::Payload extractor:

fn put_object(payload: web::Payload) -> impl Future<Item = HttpResponse, Error = ActixError> {
    web::block::<_, _, ()>(move || {
        thread::sleep(time::Duration::from_millis(10000));
        Ok(())
    }).then(|_| {
        HttpResponse::Ok().body("Upload finished")
    })
}

Memory usage when idle:

ps -o rss=,command ax | grep actix-request-body-backpressure | grep -v grep

12240 target/debug/actix-request-body-backpressure

Memory usage when waiting for response:

ps -o rss=,command ax | grep actix-request-body-backpressure | grep -v grep

63536 target/debug/actix-request-body-backpressure

[fafhrd91]

What os do you use?

[bancek]

Ubuntu 18.04.3 LTS

[fafhrd91]

well, whole algorithm is written as it is written. if you can do better PR is always welcome

[godofdream]

First of all we should update the backpressure test (https://github.com/bancek/actix-request-body-backpressure) to actix-web 2
maybe it fixed itself

[bancek]

I've updated the test to use actix-web 2.0.0 but the problem is still there.

If I understand the code correctly the request body decoding is running separately from the main request handler. So while the handler is active and reading the body there is another task/coroutine which is decoding the body and filling the buffer which the main request handler then reads. The problem is that even if the main handler is not reading the request body the decoder is still reading the HTTP stream and filling the buffer.

https://github.com/actix/actix-web/blob/3a5b62b5502d8c2ba5d824599171bb381f6b1b49/actix-http/src/encoding/decoder.rs#L102

Unfortunately I don't have enough experience with Rust and the whole async ecosystem to help you fix this problem.

[omid]

I think we should handle this like most other frameworks out there. So if the file is smaller than X bytes, keep it in memory, otherwise disk (We know disk can be also memory, based on the filesystem/OS configuration!).

For example, Spring limit is 10K by default, but it's configurable.

In some other languages like PHP, it always uses disk, which is also fine but not optimum!

[bancek]

I've upgraded to actix-web 3.0.2 and could not reproduce this.

https://github.com/bancek/actix-request-body-backpressure

[pjenvey]

I've upgraded to actix-web 3.0.2 and could not reproduce this.

@bancek et al.: any idea what changed in 3.0 to fix this?

[pjenvey]

@bancek et al.: any idea what changed in 3.0 to fix this?

robjtede suspects it could be related to #1550 since this would cause more yields