Uploading a big file fails with NoMemoryError
|Assignee:||Go MAEDA||% Done:|
Uploading of a file bigger than available RAM fails with no memory error. I've found the reason in request.raw_post which is String and doesn't respond to :read. Consequently, the whole file is read into memory. The attached patch simply replaces raw_post with body which is type of StringIO that provides read method.
Uploading a big file fails with NoMemoryError (#33752).
Patch by Karel Pičman and Pavel Rosický.
#1 Updated by Go MAEDA about 1 year ago
#2 Updated by Pavel Rosický about 1 year ago
- File attachments_test.rb.patch added
Hi @Go MAEDA,
in my opinion, the fix in #10832 is wrong. It breaks the concept of streaming file uploads https://github.com/redmine/redmine/blob/master/app/models/attachment.rb#L126
however, the proposed patch breaks FCGI. I've attached a test case.
unlike other request handlers, CGI uses a rewindable input, that doesn't support the #size method. In theory, the only way how to determine the file size is to read the content first. The content should be streamed into a tempfile not into a memory.
the second option is to rely on the CONTENT-LENGTH header, but it might not be accurate or it could be faked.
after some investigation, I found that Rails actually relies on the header
this means that FCGI is currently broken if an invalid CONTENT-LENGTH is provided. Note that most web-browsers and curl do send this header by default.
I'm wondering why anyone wants to use FCGI these days, they're definitely better options. We should try to fix it if possible, but the original patch #10832 broke other webservers that behave correctly.
Failure: Redmine::ApiTest::AttachmentsTest#test_POST_/uploads.xml_should_return_errors_if_file_is_too_big [test/integration/api_test/attachments_test.rb:207]: Expected response to be a <422: Unprocessable Entity>, but was a <201: Created> Response body: <?xml version="1.0" encoding="UTF-8"?><upload><id>24</id><token>24.1d1801f753ccd9fa57966c46f360585caf83337a394a5f238d4e4e7d6005788d</token></upload>. Expected: 422 Actual: 201 bin/rails test test/integration/api_test/attachments_test.rb:204
- File 33752.patch added
- Subject changed from Uploading big files to Uploading a big file fails with NoMemoryError
- Target version changed from 5.0.0 to 4.1.4