Ike, I actually looked at using the Apache library (instead of writing my own) when I started, but did not follow that path simply because of the potential licensing issues. However, if Jason agrees that it is okay to go down that path, I would love to replace the custom multipart parsing code with a more mature library. That being said, the multipart parsing currently relies on a library Jason wrote a while back to treat the incoming stream as multipart data. I can certainly try digging into this to see if I can pinpoint what is causing the OOM issue (if it is in the HTTP server code itself or in the multipart stream class). Thanks, --Jonathan On Fri, Apr 15, 2011 at 7:50 AM, Heiko Braun <hbraun(a)redhat.com&gt; wrote: > > > > I've run into a OOM issue when uploading content through the HTTP API > again. > (https://issues.jboss.org/browse/JBAS-9268) > > > I did take a look at the current implementation and propose that we change > it in the following way: > > - read multipart contents from mime boundary > - replace the custom multipart stream implementation with a mature one > (leans on apache commons fileupload) > > I've got this changes tested and verified in a custom branch: > https://github.com/heiko-braun/jboss-as/commits/out_of_memory > > However before going ahead, I would like get some feedback from > > a) the original author (Jonathan Pearlin. Welcome onboard btw) > b) Jason wrt the Apache License sources (See > org.jboss.as.domain.http.server.multipart.asf.*) > > > Regards, Ike > > > > > _______________________________________________ > jboss-as7-dev mailing list > jboss-as7-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/jboss-as7-dev >

The client in Question is Safari 5.0.4. I doubt is does something wrong. But you are right, most of these problems derive from the fact the multipart encoded request is wrong in some place. What I've noticed after replacing the server side pieces with the ASL ones, was that the test cases failed, because it seemed to be using an erroneous request. Beginning with the missing "boundary" delimiter as part of the content type header. AFAIK it's required. Anyway, this is not abut blaming anyone. I've just wanted to take the shortest path to desired functionality. Not sure about the ASL though. Ike On Apr 15, 2011, at 2:17 PM, Jonathan Pearlin wrote: > Ike, > > One more thing to add: when I wrote the smoke tests for the upload API, I did notice a similar issue. However, it turned out that I was not creating the multipart POST correctly (an error in how I was programmatically building the multipart was cauing the parsing to enter into an infinite loop and run out of memory). However, once I fixed the test code to produce a valid multipart post, everything was fine. In my normal development testing, I tested by uploading the Hudson WAR file (which is about 34 MB and much larger than the one you see the issue with). However, while it is possible that this issue is due to whatever is submitting the upload request not following the multipart specification, the code probably does need to be changed to handle multipart requests that do not contain completely valid data (if we cannot use the Apache library). I would be curious to see what the POST looks like that is causing the issue (in terms of the mutlipart boundary, header and payload). It is fairly easy to modify the server code to dump the incoming POST request to a file to see if it is at all different than what is expected. > > Thanks, > > --Jonathan

Yeah glancing at the code the problem is likely in the header parsing. On 4/15/11 7:17 AM, Jonathan Pearlin wrote: > Ike, > > One more thing to add: when I wrote the smoke tests for the upload > API, I did notice a similar issue. However, it turned out that I was > not creating the multipart POST correctly (an error in how I was > programmatically building the multipart was cauing the parsing to enter > into an infinite loop and run out of memory). However, once I fixed the > test code to produce a valid multipart post, everything was fine. In my > normal development testing, I tested by uploading the Hudson WAR file > (which is about 34 MB and much larger than the one you see the issue > with). However, while it is possible that this issue is due to whatever > is submitting the upload request not following the multipart > specification, the code probably does need to be changed to handle > multipart requests that do not contain completely valid data (if we > cannot use the Apache library). I would be curious to see what the POST > looks like that is causing the issue (in terms of the mutlipart > boundary, header and payload). It is fairly easy to modify the server > code to dump the incoming POST request to a file to see if it is at all > different than what is expected. > > Thanks, > > --Jonathan > > On Fri, Apr 15, 2011 at 8:07 AM, Jonathan Pearlin <jpearlin1(a)gmail.com > <mailto:jpearlin1@gmail.com>> wrote: > > Ike, > > I actually looked at using the Apache library (instead of > writing my own) when I started, but did not follow that path simply > because of the potential licensing issues. However, if Jason agrees > that it is okay to go down that path, I would love to replace the > custom multipart parsing code with a more mature library. That > being said, the multipart parsing currently relies on a library > Jason wrote a while back to treat the incoming stream as multipart > data. I can certainly try digging into this to see if I can > pinpoint what is causing the OOM issue (if it is in the HTTP server > code itself or in the multipart stream class). > > Thanks, > > --Jonathan > > > On Fri, Apr 15, 2011 at 7:50 AM, Heiko Braun <hbraun(a)redhat.com > <mailto:hbraun@redhat.com>> wrote: > > > > > I've run into a OOM issue when uploading content through the > HTTP API again. > (https://issues.jboss.org/browse/JBAS-9268) > > > I did take a look at the current implementation and propose that > we change it in the following way: > > - read multipart contents from mime boundary > - replace the custom multipart stream implementation with a > mature one (leans on apache commons fileupload) > > I've got this changes tested and verified in a custom branch: > https://github.com/heiko-braun/jboss-as/commits/out_of_memory > > However before going ahead, I would like get some feedback from > > a) the original author (Jonathan Pearlin. Welcome onboard btw) > b) Jason wrt the Apache License sources (See > org.jboss.as.domain.http.server.multipart.asf.*) > > > Regards, Ike > > > > > _______________________________________________ > jboss-as7-dev mailing list > jboss-as7-dev(a)lists.jboss.org <mailto: > jboss-as7-dev(a)lists.jboss.org&gt; > > https://lists.jboss.org/mailman/listinfo/jboss-as7-dev > > > > > > _______________________________________________ > jboss-as7-dev mailing list > jboss-as7-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/jboss-as7-dev > -- Jason T. Greene JBoss, a division of Red Hat

Standard ASL is fine. It's more keeping Deps manageable.

Thanks Heiko I'll take a look into this. Standard ASL is fine. It's more keeping Deps manageable. Sent from my iPhone On Apr 15, 2011, at 6:51 AM, Heiko Braun <hbraun(a)redhat.com&gt; wrote: > > > > I've run into a OOM issue when uploading content through the HTTP API again. > (https://issues.jboss.org/browse/JBAS-9268) > > > I did take a look at the current implementation and propose that we change it in the following way: > > - read multipart contents from mime boundary > - replace the custom multipart stream implementation with a mature one (leans on apache commons fileupload) > > I've got this changes tested and verified in a custom branch: > https://github.com/heiko-braun/jboss-as/commits/out_of_memory > > However before going ahead, I would like get some feedback from > > a) the original author (Jonathan Pearlin. Welcome onboard btw) > b) Jason wrt the Apache License sources (See org.jboss.as.domain.http.server.multipart.asf.*) > > > Regards, Ike > > > > > _______________________________________________ > jboss-as7-dev mailing list > jboss-as7-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/jboss-as7-dev _______________________________________________ jboss-as7-dev mailing list jboss-as7-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/jboss-as7-dev

Ike, I got a chance to dig into the issue this weekend and I believe I have found the cause (or causes, as the case may be). First, the code currently in the HTTP Server does not assume that there is only one body part in the multipart POST (it assumes there could be a number of fields with one of them being the actual deployment being uploaded). This was something that I did not include originally, but added later after Jason and I discussed how the upload API may need to be used in the future (i.e. what would happen if the form post needs to provide other information in addition to the deployment as part of the upload). Therefore, the code keeps reading header/body pairs from the stream until it finds one of content-type "application/octet-stream". However, upon further investigation (and using different browsers), it appears that different browsers provide different content types for the input type="file" input element in a "multipart/form-data" form (for instance, Firefox 3.x reports application/octet-stream, while IE 9 reports application/x-zip-compressed for a .war file and I suspect Safari is doing something different as well). There is definitely a defect in the logic that reads through the stream where it could never exit the outer while loop in the extractPostContent method if it never finds a part of type application/octet-stream. I have posted a patch to fix the issue here: https://github.com/jdpgrailsdev/jboss-as/commit/83f0d293e018a550562cd0381.... Note that this patch does NOT fix the issue with the different content-types -- it merely fixes the OOM and returns a HTTP 500 if we do not find a part of type application/octet-stream in the POST (see below for more thoughts on what to do with this). I have added a comment to this effect on JBAS-9268. On a related note, it appears that your patch does not handle the case described above (mutliple parts in the form post that need to be inspected to find the actual deployment -- please correct me if I am reading the code wrong!). From what I can tell, the proposed patch consumes the header (i.e. the part that contains the content-disposition and content-type declarations) and then reads the body of the part, writing it to the output stream point to the temporary file. It repeats this process until there are no more parts to read (this means that in the case of multiple parts, the bodies will be concatenated together in the output file). It *seems* as though the new patch assumes only one part for the deployment or that the deployment itself will be split across multiple boundaries. This is not the assumption that the original code was produced under (I will defer to Jason on this as regards to whether or not we need the ability to handle upload requests with multiple parts with only one of those parts being the actual deployment). I guess this leads us to a decision point as to how to modify the HTTP server: 1) Patch the existing code to a) not produce OOM when no deployment is found in the incoming POST and b) modify the code to look for the appropriate content-types that we need to support (instead of just application/octet-stream). This would mean that we want to keep the logic to handle inspecting each part of the multipart form post to find the deployment. 2) Use Ike's patch as-is. 3) Modify Ike's patch to include the logic for handling inspecting each part of the multipart form post (hybrid of 1 and 2, basically). Options 1 and 3 assume that we still need the logic to "find" the deployment in the upload POST request by interpreting the content-type of each part. Thinking about this some more, it may make sense to standardize on the name of the field instead of the type, as we can enforce it from an API perspective what people have to name the part for the API to deploy it, instead of relying on all browsers to use the same content-type for a deployment. Please let me know what you think and/or if you need me to make any of the changes! --Jonathan On Fri, Apr 15, 2011 at 7:50 AM, Heiko Braun <hbraun(a)redhat.com <mailto:hbraun@redhat.com>> wrote: I've run into a OOM issue when uploading content through the HTTP API again. (https://issues.jboss.org/browse/JBAS-9268) I did take a look at the current implementation and propose that we change it in the following way: - read multipart contents from mime boundary - replace the custom multipart stream implementation with a mature one (leans on apache commons fileupload) I've got this changes tested and verified in a custom branch: https://github.com/heiko-braun/jboss-as/commits/out_of_memory However before going ahead, I would like get some feedback from a) the original author (Jonathan Pearlin. Welcome onboard btw) b) Jason wrt the Apache License sources (See org.jboss.as.domain.http.server.multipart.asf.*) Regards, Ike _______________________________________________ jboss-as7-dev mailing list jboss-as7-dev(a)lists.jboss.org <mailto:jboss-as7-dev@lists.jboss.org> https://lists.jboss.org/mailman/listinfo/jboss-as7-dev

On 4/15/11 6:50 AM, Heiko Braun wrote: > > > > I've run into a OOM issue when uploading content through the HTTP API again. > (https://issues.jboss.org/browse/JBAS-9268) I reworked the upload as there was a number of issues. It should all be working now. I tested a number of browsers, but if you could test it as well that would be great. -- Jason T. Greene JBoss, a division of Red Hat