actix-web icon indicating copy to clipboard operation
actix-web copied to clipboard

Hang or protocol error when sending early response to a POST request with delayed body

Open Riateche opened this issue 3 years ago • 7 comments

Expected Behavior

This is a minimal example of a client that reproduces the problem:

[dependencies]
tokio = { version = "1.0", features = ["full"] }
reqwest = { version = "0.11", features = ["stream"] }
futures = "0.3.16"
use futures::stream;
use std::time::Duration;
use std::{env, io};
use tokio::time::sleep;

#[tokio::main]
async fn main() {
    let client = reqwest::Client::new();
    loop {
        let stream = stream::unfold(0u32, |state| async move {
            if state == 0 {
                sleep(Duration::from_secs(1)).await;
                println!("yield data");
                Some((io::Result::Ok(&b"abc"[..]), state + 1))
            } else {
                println!("yield none");
                None
            }
        });
        let response = client
            .post(env::args().nth(1).unwrap())
            .body(reqwest::Body::wrap_stream(stream))
            .send()
            .await
            .unwrap();
        println!("status: {:?}", response.status());
        let bytes = response.bytes().await.unwrap();
        println!("body: {:?}", bytes);
        println!("========");
        sleep(Duration::from_secs(3)).await;
    }
}

The client sends a POST request with a body, but the data for the body is not available right away. If the server does not respond with an error right away, the client will send the body as it becomes available, then the server will return the response:

cargo run -- https://httpbin.org/post 
yield data
yield none
status: 200
body: b"{\n  \"args\": {}, \n  \"data\": \"abc\", \n  \"files\": {}, \n  \"form\": {}, \n  \"headers\": {\n    \"Accept\": \"*/*\", \n    \"Host\": \"httpbin.org\", \n    \"Transfer-Encoding\": \"chunked\", \n    \"X-Amzn-Trace-Id\": \"Root=1-610f0968-7e9f910541c7bc430ad6928e\"\n  }, \n  \"json\": null, \n  \"origin\": \"94.72.10.210\", \n  \"url\": \"https://httpbin.org/post\"\n}\n"
======== (repeats forever)

The server can also return an error right away, in which case the client doesn't poll the body stream at all:

cargo run -- https://httpbin.org/404 
status: 404
body: b"<!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 3.2 Final//EN\">\n<title>404 Not Found</title>\n<h1>Not Found</h1>\n<p>The requested URL was not found on the server.  If you entered the URL manually please check your spelling and try again.</p>\n"
======== (repeats forever)

Some servers can return the status immediately but still read the request body before sending the response body. This is the default behavior for nginx on a 404 page:

cargo run -- http://127.0.0.1/404
status: 404
yield data
yield none
body: b"<html>\r\n<head><title>404 Not Found</title></head>\r\n<body>\r\n<center><h1>404 Not Found</h1></center>\r\n<hr><center>nginx/1.20.1</center>\r\n</body>\r\n</html>\r\n"
======== (repeats forever)

Either way, the client should read the response status and body successfully.

Current Behavior

I've tested it on web-v4.0.0-beta.8 tag with cargo run --example basic.

Original example code, 404 page:

cargo run -- http://127.0.0.1:8080/404  
status: 404
body: b""
========
yield data
yield none
yield data
yield none
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: reqwest::Error { kind: Request, url: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Ipv4(127.0.0.1)), port: Some(8080), path: "/404", query: None, fragment: None }, source: hyper::Error(IncompleteMessage) }', src/main.rs:25:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

After the first response is returned, the client still attempts to send the body for the first response (even though the request object no longer exists) which doesn't seem intended. The second request returns an error. The actix-web server doesn't log the second request at all.

The behavior changes when keepalive is enabled in the example code (.keep_alive(120)):

cargo run -- http://127.0.0.1:8080/404
status: 404
body: b""
========
yield data
yield none
yield data
yield none

(The client hangs forever.)

If the endpoint exists and it reads the body before responding, everything works fine. This handler code works:

#[post("/post1")]
async fn post1(payload: web::Bytes) -> &'static str {
    println!("payload: {:?}", payload);
    "post1 ok\r\n"
}

However, if the body is retrieved lazily and the handler doesn't read the body, the bug is reproduced. This handler reproduces the bug:

#[post("/post1")]
async fn post1(payload: web::Payload) -> &'static str {
    "post1 ok\r\n"
}

Steps to Reproduce (for bugs)

  1. Run actix-web server: cargo run --example basic
  2. Run the example client posted above: cargo run -- http://127.0.0.1:8080/404

Context

I'm trying to implement an endpoint for multipart file upload. If the request doesn't pass some initial checks, I want to return an error response early without reading the (potentially large) body. That will allow the client to cancel the body upload and save time and bandwidth.

Your Environment

  • Rust Version (I.e, output of rustc -V): rustc 1.53.0 (53cb7b09b 2021-06-17)
  • Actix Web Version: 4.0.0-beta.8

Riateche avatar Aug 07 '21 23:08 Riateche

@Riateche thanks for the detailed report, it was very helpful in figuring out the cause.

robjtede avatar Feb 04 '22 01:02 robjtede

Reopening since the fix was reverted

Sytten avatar Jun 21 '22 17:06 Sytten

+1, was about to report this until I stumbled on this report. Experiencing the same issue, have to read out the payload:

while let Ok(Some(mut field)) = payload.try_next().await {}

Or the client connection hangs sporadically and sometimes indefinitely. In the interim is there a graceful way to interrupt the stream and terminate the client connection?

chriskyndrid avatar Jul 30 '22 05:07 chriskyndrid

+1, any news on this?

EdMcBane avatar Nov 07 '22 16:11 EdMcBane

+1, this is very reproducible in my environment. I've handling file upload via POST. In some cases, I want to abort the upload by

return Ok(HttpResponse::NotAcceptable().body("Invalid file type."));

after 2 bad requests, the client will hang. I can work around it by reading the entire incoming stream, but I'd say it's not ideal, especially if a large file is POSTed.

while let Some(chunk) = field.next().await {};
return Ok(HttpResponse::NotAcceptable().body("Invalid file type."));

ctacke avatar Nov 10 '22 16:11 ctacke

Any updates on this? The issue has been moved out quite a few times over the past few years, is a fix still expected to be implemented in the 4.4 release?

chriskyndrid avatar Jan 23 '24 15:01 chriskyndrid