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 broken with AsyncRead #1380

Closed
Meeuwisse opened this issue Jul 20, 2020 · 4 comments
Closed

Multipart broken with AsyncRead #1380

Meeuwisse opened this issue Jul 20, 2020 · 4 comments
Labels
question A question (converts to discussion)

Comments

@Meeuwisse
Copy link

I'm running a little internal web service on nightly - there's some features I'm using that aren't yet in stable. Trying to stay up to date with the latest, and am a little stumped on recent async changes.

For some administrative functionality I'm using Multipart 0.16.1. I see there's 0.17.0 and possibly Multipart-async. Before I try those, I'd like to have matters working again. Or else I would lose sight of what's causing problems.

This endpoint is pretty much a one-off and won't be called regularly, so performance isn't at all a priority. I had something along the lines of;

#[post("/", data = "<data>")]
fn post(cont_type: &ContentType, data: Data) -> String {
	if !cont_type.is_form_data() {
		return "Wrong contenttype".to_owned();
	}

	let (_, boundary) = match cont_type.params().find(|&(k, _)| k == "boundary") {
		Some(x) => x,
		None => return "No boundary".to_owned()
	};

	match Multipart::with_body(data.open(), boundary).save().memory_threshold(0).temp() {
		Full(entries) => {
			// Further data processing here
		},
	        Partial(_, _) => {
        	    "Partial".to_owned()
	        },
	        Error(e) => e.to_string()
	}
}

This now gives me;

    = note: the method `save` exists but the following trait bounds were not satisfied:
            `rocket::data::DataStream: std::io::Read`

And I see in 5d439ba this got dropped;

- impl Read for DataStream {
+ impl AsyncRead for DataStream {

Now, figuring this out took a little longer than expected so I was hoping for a quick fix based on Shepmaster's approach;

	// data stopped supporting Read trait, copy from AsyncRead
	let mut d = Vec::new();
	block_on(data.stream_to(&mut d)).expect("Unable to read");

	match Multipart::with_body(Cursor::new(d), boundary).save().memory_threshold(0).temp() {

Except this sadly doesn't seem to work; block_on never returns. I'm still reading up on how the async/await functionality works, and am afraid I'm just out of my depth here. How do I get this going again? Much obliged.

@jebrosen
Copy link
Collaborator

Yes, the bounds were changed here. Off the top of my head I think that multer is a futures-based multipart library, so it should be possible to use that. I haven't looked at any details yet, though.

The "read everything to a Vec first, then use a synchronous multipart parser" is still possible, but discouraged for the same reason as before: stream_to will accept an infinite amount of data from the client directly into memory. It's still useful for getting something working in the short term, of course.

Except this sadly doesn't seem to work; block_on never returns. I'm still reading up on how the async/await functionality works, and am afraid I'm just out of my depth here. How do I get this going again? Much obliged.

block_on is the wrong thing to use here - see https://github.com/SergioBenitez/Rocket/blob/master/site/guide/3-overview.md#cooperative-multitasking. In this case, I assume that block_on is waiting indefinitely for more data to be available while the code that actually reads the data from the network can't run until the next .await point -- i.e. a deadlock.

Instead, you should use data.stream_to(&mut d).await.expect("Unable to read");. This will also mean you need to change fn post to async fn post.

If you are still having trouble after that, please ask! This is all really helpful feedback that can help inform the migration guide for 0.5.

@Meeuwisse
Copy link
Author

I actually hadn't tried turning it all into async yet. That gave me a few other minor problems, mostly because I simplified my example somewhat. I feel silly now for not starting with that. It does work, so thank you.

The documentation you referred to mentions a form of cooperative multitasking and warns for mutexes. I actually use MutexGuard quite a bit, and earlier example does a save() to further massage data into a temporary file (for Calamine). Are these warnings Rocket-specific or generally applicable to Rust's async/.await functionality? Is it avoidable as long as my functions remain non-async, or are there good alternatives? I'd rather tackle them now, instead of an inevitable bug report that the server is unresponsive. Thanks!

@jebrosen
Copy link
Collaborator

Are these warnings Rocket-specific or generally applicable to Rust's async/.await functionality? Is it avoidable as long as my functions remain non-async, or are there good alternatives?

They apply to just about all code using Rust's async/await feature. There are of course always exceptions, but in general blocking in async code is problematic. For example a (synchronous) MutexGuard can actually be fine, as long as it is short-lived and does not exist across an .await.

Declaring a route as a fn instead of async fn does not actually make any difference for these purposes; that can definitely be explained a bit better in the documentation I linked.

@jebrosen jebrosen added the question A question (converts to discussion) label Jul 20, 2020
@SergioBenitez
Copy link
Member

Looks like this has been resolved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question A question (converts to discussion)
Projects
None yet
Development

No branches or pull requests

3 participants