File upload bug still present in rc3

[chamcha]

hi developers,
the bug is at org.jboss.nukes.servlet.MultipartRequest.java line 68.

I copied here the code of MultipartRequest constructor, please read my comment (in bold):

public MultipartRequest(HttpServletRequest request, int sizeMax)
throws FileUploadException
{
super(request);

// Create a new file upload handler
DiskFileUpload upload = new DiskFileUpload();

// Set upload parameters
upload.setSizeMax(sizeMax);

// the next statement is wrong because setSizeThreshold
// sets the size limit of the memory stream upload (read apache docs)
// not the max size of the upload.
// Actually, if sizeMax=30MB all files <= 30MB are uploaded in memory!

upload.setSizeThreshold(sizeMax);
upload.setRepositoryPath(System.getProperty("java.io.tmpdir"));

// Get request parameters
for (Enumeration en = request.getParameterNames();en.hasMoreElements();)
{
String name = (String)en.nextElement();
parameters.put(name, request.getParameterValues(name));
}

// Merge with upload fields
for (Iterator i = upload.parseRequest(request).iterator();i.hasNext();)
{
FileItem item = (FileItem)i.next();
if (item.isFormField())
{
parameters.put(item.getFieldName(), new String[]{item.getString()});
}
else
{
files.put(item.getFieldName(), new UploadedFile(item.getContentType(), item.get()));
}
}
}

[theute]

I made a fix, we don't distribute it though, in the wiki page, take the commons-upload.jar

The bug is in the Apache library, it allocates 30MB whatever the size of a field is, stupid huh ?

We read the documentation, i also read Apache code.

Files are not written on disk but kept in the database for now.

Check this thread: http://www.jboss.org/index.html?module=bb&op=viewtopic&t=52640

[chasetec]

To quote you from the thread you mentioned :)

commons-fileupload.jar from Apache of course (not commons-upload.jar as i wrote)

And thanks Thomas for the patched version. I missed that wiki page at first and was trying to figure out why previewing a simple text message on a forum was sucking up a hundred megs of memory. You weren't kidding when you titled that page

Submitting a form with a file depletes memory

Why isn't the patched version distributed? Is it simply because the thirdparty cvs directory is shared with other JBoss projects?

-Chase

[chamcha]

"theute" wrote:

The bug is in the Apache library, it allocates 30MB whatever the size of a field is, stupid huh ?

I'm sorry theute, but I don't agree. The fact that the persistent storage is in db, is uninfluent. I think that temporary files are better that memory buffer for temporary storing such large files. So I prefer the original commons-fileupload code with a low threshold (10-15KB).

[theute]

Did you see how is the original one ?

My modification is very simple and wouldn't change what you want do to. 15KB is way too low, opening a file, writing in it and closing a file take too much time if your file is 30KB you are doomed.

If you put a threshold of 150KB then allocating that much memory for a text field of 0 character is still non-sense to me.

Storage in DB is influent, if we were stocking in a file then we would have the cost to write in a file at some point, in our case we don't.

Your example was about 30MB, HTTP is not made for uploads like this, for large uploads, FTP is recommended for any web server.

I agree there may have a problem but the solution is not as simple as you say.

I got people asking for my fix working on other projects that Nukes. I read people complaining about that problem on forums as well. We are not alone coping with that problem.

In a perfect world it works and it's just a little bit faster but i don't know any project working in a perfect world.

A solution might be to get rid of commons-upload and do the parsing by ourself. It is not a priority right now.

[chamcha]

"theute" wrote:

My modification is very simple and wouldn't change what you want do to. 15KB is way too low, opening a file, writing in it and closing a file take too much time if your file is 30KB you are doomed.

too much time? this is a file upload, not a real time application. Another point: with a 30MB file upload and memory buffer, how many times the initial buffer of 32 bytes must be reallocated? That is quite expensive, I think.

"theute" wrote:

Your example was about 30MB, HTTP is not made for uploads like this, for large uploads, FTP is recommended for any web server.

I agree, but http is much simple from the user's point of view.

"theute" wrote:

In a perfect world it works and it's just a little bit faster but i don't know any project working in a perfect world.

Why the apache group refused your code?

Regards,
Marco

[julien1]

"chamcha" wrote:

"theute" wrote:

My modification is very simple and wouldn't change what you want do to. 15KB is way too low, opening a file, writing in it and closing a file take too much time if your file is 30KB you are doomed.

too much time? this is a file upload, not a real time application. Another point: with a 30MB file upload and memory buffer, how many times the initial buffer of 32 bytes must be reallocated? That is quite expensive, I think.

The problem here is not performance wise, the problem is that it generates out of memory exceptions.

"chamcha" wrote:

"theute" wrote:

Your example was about 30MB, HTTP is not made for uploads like this, for large uploads, FTP is recommended for any web server.

I agree, but http is much simple from the user's point of view.

This is CMS, we cannot ask everyone to use an FTP client.

"chamcha" wrote:

"theute" wrote:

In a perfect world it works and it's just a little bit faster but i don't know any project working in a perfect world.

Why the apache group refused your code?

We did not submit it (yet), but remy maucherat main lead of tomcat is aware of the issue (we told him).

[theute]

"chamcha" wrote:
too much time? this is a file upload, not a real time application. Another point: with a 30MB file upload and memory buffer, how many times the initial buffer of 32 bytes must be reallocated? That is quite expensive, I think.

Writing on disk and in memory we are talking about 2 different scales. Plus the buffer should double each time, and again 30MB over HTTP is not serious.

"chamcha" wrote:
I agree, but http is much simple from the user's point of view.

I don't know cases where a user need to upload more than 2 or 3MB of data. Again, otherwise this is dangerous for any web server.

"chamcha" wrote:
Why the apache group refused your code?

I didn't submit mine, they refused another code on the same problem,
http://issues.apache.org/bugzilla/show_bug.cgi?id=24306
this thread is interesting too: (i like that sentence in it:
"Boss says it has to be delivered via HTTP. Cannot use FTP, no matter how much I beg."
http://forums.java.sun.com/thread.jsp?thread=448639&forum=31&message=2034372

Anyway there is no perfect solution for everybody. It depends also on the purpose of the website (ie: if there is lot of uploads, the size of an average upload, the size of the maximum upload size...)

For now the new fileupload.jar fixed troubles for people who had some. I don't like the way it is but it works, this is not perfect but again this is not a priority.
If you have a great solution that work for anybody, please submit a patch and i will be glad to commit it for you.

[chamcha]

I agree with Julien, the problem is not about performances. The problem is that nukes uses a modified version of a very common library: commons-fileupload.

So, nukes uses DiskFileUpload class, that extends FileUploadBase. Why don't you make a custom realization of FileUploadBase, instead of using DiskFileUpload?

Regards,
Marco