JSPWIKI-1205 adjusts the handling for too large file attachments. #405
Conversation
…s change, while it addresses the issue, might cause a new issue whereby updatlong an extremely large file might cause OOM errors since the commons file uploader parses and buffers the whole http request. And the way the upload page works, it is not possible to get the correct redirect url without parsing the request. unformately, i don't think there's a way to add an http header to multipart form post from javascript.. open to other opinions on this
… that provides a list of server installed plugins and alters the UI to support fetching and displaying them. Alters the Plugin API definition to have two additional methods with default implementations that should hopefully support backwards compat Alters the PluginManager API to include a getDiscoveredPlugins Alters a few of the javascript mechanisms to get the plugin list ti display with the snip suggestions. This seems to cause a duplicate of the IfPlugin Adds a test hello world type plugin to verify this functionality
…endpoint that provides a list of server installed plugins and alters the UI to support fetching and displaying them." This reverts commit 0df4e22.
|
i'm thinking resource exhaustion is an issue with this change set. let me make a few edits and some testing |
…s now redirect correctly and show the too big error message in Error.jsp
spyhunter99
added
the
review requested
this should be a non issue currently. the max file upload property now checks for the entire http request so you can upload as much as you want as long as the http request isn't better that size + the existing checks on per file size still holds. if you hit the max, you get a more suitable error message now |
juanpablo-santos
left a comment
juanpablo-santos
left a comment
There was a problem hiding this comment.
one note on chunked file uploads
|
|
||
| m_engine.getManager( ProgressManager.class ).startProgress( pl, progressId ); | ||
|
|
||
| if ("chunked".equalsIgnoreCase(req.getHeader("Transfer-Encoding"))) { |
There was a problem hiding this comment.
I'd rather let the container try to save the file in this case, but that's only one opinion. Related to this, perhaps displaying the attach file limit on the upload page would help to alleviate this kind of errors?
There was a problem hiding this comment.
fair point. in testing, i couldn't get the chunked encoding to happen with any browser or file size in this case so i think this is a 'just in case' scenario.
good idea putting the limit on the upload page, that should be doable
There was a problem hiding this comment.
let the container try to save the file
So reading the docs for commons file upload what we currently have is correct for handling large files with memory efficiency. I must have misinterpreted or missed the fact that we are using the disk item factory for this. I'll double check everything but i think we can remove the chunked encoding part. sounds like the servlet container takes care for that for us anyhow
# Conflicts: # jspwiki-main/src/main/java/org/apache/wiki/attachment/AttachmentServlet.java # jspwiki-war/src/main/webapp/Error.jsp
2 participants
Root issue is that when uploading a large file (larger than the configured max), the browser gets a 302 redirect, however in my experience, it just displays a connection closed message. The response looks like it should be ok otherwise.
Anyhow, the fix is to ensure that the uploader rejects the files that are too big and redirects to the upload attachment page by simply moving the logic around in the servlet.
This change, while it addresses the issue, might cause a new issue whereby uploading an extremely large file might cause OOM errors since the commons file uploader parses and buffers the whole http request. And the way the upload page works, it is not possible to get the correct redirect url without parsing the request. unfortunately, i don't think there's a way to add an http header to multipart form post from javascript. open to other opinions on this or if the risk is worth it.