allow overriding the rack multipart parser tempfile class #639
Conversation
|
This seems fine. We should merge this @raggi . |
|
Seems good, I will merge it. But let's wait first a little while for @raggi to review it since he was pinged :). |
| @@ -209,7 +214,7 @@ def get_data(filename, body, content_type, name, head) | |||
| # filename is blank which means no file has been selected | |||
| return | |||
| elsif filename | |||
| body.rewind | |||
| body.rewind if body.respond_to?(:rewind) | |||
There was a problem hiding this comment.
Does the spec currently allow body to not respond to rewind?
I've always had two minds about enforcing rewind in the spec as it adds hard requirements for file buffering or hard bounds from memory pressure - opening up a large class of issues for users. It's also a problem for integration in sandboxed environments, particularly those that would prefer to have read only filesystems and so on.
What is even more painful is that many FEs are already buffering to disk, and so we're pumping this data in and out of the kernel in manners all the time. I'd love for this to be better.
Anyway, discourse over - back to the point: can you check for me on the state of this with regard to SPEC, and if you want to propose a change there we should discuss.
There was a problem hiding this comment.
Keep in mind that body here isn't the request body, it's the Tempfile (or with this change, the custom class) created above. So it's purely a stream internal to Rack::Multipart::Parser. As far as I can see, that's not touched on by the rack spec at all.
There was a problem hiding this comment.
Oh, yes, we're over here. Apologies, I was thinking of the request body rules and only glancing.
|
I like this in principle, want to check the SPEC details to be clear on what we're affecting, and then happy to merge. |
|
Note to commiter: we need to add this to SPEC as an optional server feature. |
|
@raggi I took a stab at implementing the SPEC and lint changes just based on the surrounding code in those files. Let me know if anything needs tweaking, or if I'm barking up entirely the wrong tree. |
|
@raggi any thoughts on that last addition? |
|
Would you mind rebasing against master? |
This allows for more flexibility in how to buffer (or stream) multipart file uploads, rather than always using Tempfile and buffering to local TMPDIR.
|
@raggi: rebased and the travis build is passing |
|
Thank you all! |
|
oh sweet, thanks |
|
no, no, thank you! ❤️ |
This allows for more flexibility in how to buffer (or stream) multipart file uploads, rather than always using Tempfile and buffering to local TMPDIR.
Also allow a custom buffering size, to optimize large file reads.
Our initial use case for this is to use it in combination with Passenger's new PassengerBufferUpload feature in order to allow a Rails action to stream multipart file uploads to S3 as they are being uploaded by the client, with absolutely no local copy stored at all.
Our initial solution was to make a custom
Requestsubclass that duplicated theRack::Multipart::Parsercode just to change this singleTempfile.newline, but the functionality seems generally useful so I decided to do a pull request.Let me know if using these
envvariables is the best way to pass this information to the parser, it seemed like the most flexible and least invasive solution.