I think that we should get rid of body_io on both client response and server request. The body of a request should be an IO to force developers to think of the correct way to handle requests efficienctly. Legitimising reading http requests in as strings may create an environment where crystal webapps treat http bodies as strings first and IOs later, which is both inefficient and dangerous to memory.

@RX14 I think for an HTTP client this is less dangerous. For example if I want to consume GitHub's API to fetch a repo's info, the string will be relatively small so that won't be a problem. In those cases getting the body as a string is pretty convenient. And parsing JSON from a small string is much faster than from an IO, mostly because you can create string subslices from the original string, but with an IO you can't (at least easily) do that.

For a server it is a bit more dangerous because someone can send a big body and you can't control that. So in that case I agree that only body as an IO should be provided.

Unfortunately then you have an inconsistency. I would argue that the body object of every {server,client} {request,response} should be of the same type so that all types follow the same basic interface. Having wildly different interfaces for request and response depending on if it's client or server violates the principle of least surprise. Maybe both Response types should include a HTTP::Request module which provides basic common methods.

I would also argue that the body should be read-once, and request and response types be immutable structs. This has performance advantages but also makes the implementations much simpler without having to synchronise many different fields with mutable caches.

@asterite i can confirm that the memory stays the same. Even for really big files (1GB) for multipart parsing.

However the parsing got 4x slower with this patch.

Before: 6324.3 ms
After: 26427.92ms

WDYT?

@sdogruyol Cool! Parsing of what? :-)

@asterite parsing of a 1 GB file (ubuntu-14.04-desktop.img) upload with multipart 😄

@sdogruyol I think it's expected that it's going to be slower, because parsing from a string or MemoryIO is faster than from an IO over a socket. However, it shouldn't be that slower. I profiled the code and found a few places where I could optimize things. Could you try again? (warning: I forced pushed). I don't think it will be equally fast as before, but maybe 2x slower or similar...

Using a larger buffer size in IO.copy might help. 8KB is probably a good size to fit in L1 cache.

@asterite

Before: 26427.92ms
Now: 12437.06ms

More than 2X faster 🎉 But still 2 times slower than the previous one 😿

@sdogruyol Try using:

buffer = uninitialized UInt8[8192]
while (len = src.read(buffer.to_slice).to_i32) > 0
  dst.write buffer.to_slice[0, len]
end

instead of IO.copy, where src is the multipart IO and dst is the file.

BTW i do think that this is pretty good for 1.0. After that we can think of a way to speed it up more.

Closed in favor of #3406