Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multipart data stream hangs forever if the "payload: Multipart" is moved from the function that process the route #2457

Closed
nicoan opened this issue Nov 23, 2021 · 4 comments · Fixed by #2463
Labels
A-multipart project: actix-multipart C-bug Category: bug

Comments

@nicoan
Copy link

nicoan commented Nov 23, 2021

Expected Behavior

actix-multipart's file fields should yield all the bytes streamed until it's done

Current Behavior

actix-multipart's file fields gets stuck awaiting data forever

Possible Solution

I don't have a possible solution, sorry

Steps to Reproduce

Running this code should be enough to reproduce the bug!

use std::{fs, io::Write};

use actix_multipart::{Field, Multipart};
use actix_web::{web, App, HttpResponse, HttpServer, Responder};
use futures_util::stream::StreamExt;

#[actix_web::main]
async fn main() -> Result<(), actix_web::Error> {
    fs::create_dir_all("./uploads").expect("cannot create profile pics directory");

    HttpServer::new(move || App::new().route("/upload", web::post().to(upload)))
        .bind("127.0.0.1:8000")?
        .run()
        .await?;

    Ok(())
}

// If we change this to `mut payload: Multipart`...
async fn upload(payload: Multipart) -> impl Responder {
    // ...and here to &mut payload, it works.
    let photo = get_multipart_field("photo", payload).await;
    if let Some(mut field) = photo {
        let mut file = web::block(|| fs::File::create("./uploads/test"))
            .await
            .unwrap();

        // Here is where it gets stuck
        println!("Before getting data");
        while let Some(chunk) = field.next().await {
            println!("This never prints!");
            let data = chunk.unwrap();
            file = web::block(move || file.write_all(&data).map(|_| file))
                .await
                .unwrap();
        }
        HttpResponse::Ok()
    } else {
        HttpResponse::BadRequest()
    }
}

async fn get_multipart_field(
    name: &str,
    mut payload: Multipart, // If we change this to `payload: &mut Multipart`, it works
) -> Option<Field> {
    while let Some(field) = payload.next().await {
        let field = field.unwrap();
        if let Some(content_disposition) = field.content_disposition() {
            match content_disposition.get_name() {
                Some(field_name) if name == field_name => return Some(field),
                _ => continue,
            }
        }
    }

    None
}

You can hit the endpoint with this curl:

curl -i -X POST  -F photo=@"<PATH_TO_A_FILE>"  http://127.0.0.1:8000/upload

Context

I was just testing how to upload a file using actix-multipart

Environment

  • OS: Ubuntu 20.04.03
  • Rust Version: rustc 1.56.0 (09c42c458 2021-10-18)
  • Actix Web Version: 3.3.2
  • Actix Multipart version: 0.3.0
  • futures-util: 0.3.17
@asonix
Copy link
Contributor

asonix commented Nov 23, 2021

From a quick glance: I don't think the problem is with moving payload. I think the problem is with dropping payload prematurely.

When you move payload into the function, it drops when the function ends, and then you're trying to extract chunks from the field stream after payload is gone already.

It's not intuitive that the field stream is tied to the payload stream in this way

@robjtede robjtede removed the v3.x label Nov 23, 2021
@fakeshadow
Copy link
Contributor

maybe filed should be referencing the payload to prevent misuse

@robjtede robjtede added this to the actix-web post-v4 milestone Nov 23, 2021
@aliemjay
Copy link
Member

The original code intended to handle this case by failing immediately instead of hanging. #2463

I won't argue for this behavior, but this is the eaziest way to be fixed until we figure out the proper way for synchronizing Multipart and Field and whether we want to allow them to be spawned to separate tasks.

@robjtede robjtede added C-bug Category: bug A-multipart project: actix-multipart labels Nov 26, 2021
@robjtede
Copy link
Member

robjtede commented Dec 1, 2021

released in actix-multipart v0.4.0-beta.9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multipart project: actix-multipart C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants