Actix Multipart Panics & Closes Connection on Early Response

[Geometrically]

Expected Behavior

In a Multipart request, as parts are received a server may validate it, check the size for maximums, and more. If an error occurs and a user need to know about it, the server can issue an "early" response, where the other parts are ignored.

Current Behavior

Actix-web returns an internal server error, closes connection, ends up with no content even though content was specified, and the worker panics with the message: thread 'actix-rt|system:0|arbiter:1' panicked at 'Multipart polled after finish'

Possible Solution

https://github.com/actix/actix-web/blob/03456b8a33a4550b94785a7612e16d715755cd00/actix-http/src/h1/dispatcher.rs#L739

This line of code on the actix web server should change. Requests should not return an error if part of the payload is dropped.

Steps to Reproduce (for bugs)

https://github.com/Geometrically/actix-web-bug-poc Run the actix server above, and request using CURL.

curl -i -X POST http://127.0.0.1:8080 \ 
   --header 'Content-Type: multipart/form-data; boundary=---------BOUNDARY' \
   --data-binary @file.txt

The expected output is: this does not show up :( However, no content is returned, and a panic is printed in the console. thread 'actix-rt|system:0|arbiter:1' panicked at 'Multipart polled after finish'

Context

My website, https://modrinth.com uses actix-web to power our open-source API (https://github.com/modrinth/labrinth). In our website, users can upload mods which have many files. We use multipart requests to do this. However, files are validated, so if an error occurs in validation it is returned early. Instead of returning the error, actix-web panics, causing the connection to close, so no error shows up to the user. Because of the panic, we cannot rollback steps in the project creation process.

Your Environment

See minimum reproducible example, barebones actix environment with actix-multipart 0.4.0

• Rust Version: 1.59.0
• Actix Web Version: 4.0.1/4

[Geometrically]

My minimum reproducible example might not return the correct error. In production, this error is printed: [2022-03-15T18:12:05Z ERROR actix_http::h1::dispatcher] handler did not read whole payload and dispatcher could not drain read buf; return 500 and close connection

[fakeshadow]

It's wrong to blindless return internal error when paylaod is half consumed. Instead it should alter the connection state to close if necessary.

[fakeshadow]

The following is not directly related to this issue but I believe at lower level the question is: Should actix-web handle situational connection close magically or just let it go and trust higher level(http response in this case)?

Ideally user should have control if they want to close a connection or not. In this case an overflow request body too large for the server's limit. This is achievable with simple Connection: Close header and with it actix-web knows what to do(Drain the half consumed body if connection is kept alive and stop reading if closed).

actix-web does offer related api to do it but it does not reinforce it when user building response(Include both ResponseBuilder type and ResponseError trait).

[akeamc]

I am also facing this issue when implementing my own multipart handler (because I need multipart/related specifically). If the request payload is large and an error is returned halfway through reading it, the connection is sometimes reset (and an error message is logged: handler did not read whole payload and dispatcher could not drain read buf; return 500 and close connection) and sometimes I get the specified HTTP response.

[Victor-N-Suadicani]

I just ran into this error in production. On large uploads, I deliberately stop reading the payload early if the user is already over the upload limit (why would I continue reading it after all?). Then I get: handler did not read whole payload and dispatcher could not drain read buf; return 500 and close connection despite my code continuing to produce a valid response.

Any pointers as to how this can be fixed?

[ModProg]

I have a .for_each(|_| ready(()).await before all the returns. But that is probably not ideal (got the idea from https://github.com/rust-lang/futures-rs/issues/707)