Repository: netty/netty
Issue: MixedFileUpload in Netty 3.5.0 snapshot doesn't write out entire file.

I duplicated the functionality of file upload handler in the example for my application.

My HttpDataFactory was done the same way as the example, where I would have a certain size which would be in memory only, but it it got larger, it would be on disk. I also had no HttpChunkAggregator in my pipeline.

HttpDataFactory factory = new DefaultHttpDataFactory(
            DefaultHttpDataFactory.MINSIZE);

This worked great for small files, an I was able to take a small file upload and use FileUpload.moveTo() to put it elsewhere in the file system.

However, if I did a much larger file, say several meg, the file from FileUpload.moveTo()  would be missing about 10K of the file. However, FileUpload.length() would give the correct file length. The subclass of the FileUpload was MixedFileUpload.

When I switched to disk-only

HttpDataFactory factory = new DefaultHttpDataFactory(true);

the FileUpload.moveTo() would give me all of the bytes.

I would assume the expected behavior for Mixed would be to get the entire file. I didn't have time to dig too deeply, perhaps the memory upload used for the first few chunks that fit under the minimum are not first written to the file when it switches over to Disk.

After changing to disk only I was able to upload 4 Gig files, no problem.

Thanks for adding this functionality, it was great.


Issue comments:
Hi @robotbrains 

Concerning the moveTo, is it renameTo() you talk about ? (I suppose yes)

I would recommend to not use HttpChunkAggregator, since one of the interest of this upload feature is to limit as much as possible the memory impact. Having such a HttpChunkAggregator in your pipeline will put all the file in memory before putting it on disk, which is then unuseful (if it fits of course).

Now on your behavior, it should not give you only the file -10K when renameTo is used.
Indeed, FileUpload.length returns the number of bytes written to the underlying file (or buffer if in memory).
When using a MixedFileUpload (through DefaultHttpDataFactory with a limit), it starts with a MemoryFileUpload until it reaches the limit size (about 16KB), then writes all the previous bytes to a new DiskFileUpload, then writes the extra new bytes that will oversize the limit.
On my side, I did not see such issue. However I most of the time use no HttpChunkAggregator, but I do not know if this might be the issue.

If you have an example of your issue, I would try to see where it should come from.
It might a be a bug...

Again me, I just try by adding a HttpChunkAggregator (with huge size allow) to check, and it works on my side too.
Note however that when I try to get very huge file, I get a heapspace memory issue on the server side (due to the aggregator).
If I removed the aggregator, then everything is ok again, whatever the size (and memory footprint still limited to few KB).

If you have an example where we can reproduce the error, we will surely fix it.

My message said it had no aggregator. I need to be able to handle very large files and don't want to run out of memory.

Yes, it is renameTo(), not moveTo().

It should copy from the memory, but doesn't in the drop I have, which is from Monday this week.

-Keith

Sorry, I read too quickly your message. Indeed, no Aggregator.

I will get the current 3.5 to see if I can reproduce the behaviour.
By the way, do you use a Java client to send the file, or do you use a web browser and then which one ?

I use Chrome. I also tested and got the same behavior with Firefox, wanted to make sure it wasn't a Chrome bug.

Chrome Version 19.0.1084.41
Firefox 12.0

I would have loved to respond with a patch, but am crushed for time right now, so once I found the disk only solution worked I quit looking in the code.

I have found the reason. One of the patch was not applied.
I am currently making the necessary patches.

Thank you for reporting this issue !

I think you can download again the last revision and check if it is ok now (I have tested on my side of course).
Then if it is ok, you could close this issue.

OK, will check later today. Thanks!

@robotbrains did you have time yet to check ?

Checking now.

I can't compile.

software/netty/netty-netty-80a2ae6/src/main/java/org/jboss/netty/buffer/CompositeChannelBuffer.java:[278,12] error: cannot find symbol

That is all I got from Maven. The code has 

DetectionUtil.javaVersion() >= 7

on that line. No import.

Adding that in for my test, but it should be fixed.

I just fixed this.. That was my fault!

Everything works great now. Many thanks!


Pull request review comments:
This will break compatiblity with java5

This will break compatiblity with java5

This will break compatiblity with java5

This will break compatiblity with java5

This will break compatiblity with java5

Ah, I see !
Understood


Patch that resolved the issue:
diff --git a/src/main/java/org/jboss/netty/handler/codec/http/MixedAttribute.java b/src/main/java/org/jboss/netty/handler/codec/http/MixedAttribute.java
index 292b0bc4d17..caad1b492fb 100644
--- a/src/main/java/org/jboss/netty/handler/codec/http/MixedAttribute.java
+++ b/src/main/java/org/jboss/netty/handler/codec/http/MixedAttribute.java
@@ -64,7 +64,7 @@ public void addContent(ChannelBuffer buffer, boolean last) throws IOException {
                         .getName());
                 if (((MemoryAttribute) attribute).getChannelBuffer() != null) {
                     diskAttribute.addContent(((MemoryAttribute) attribute)
-                        .getChannelBuffer(), true);
+                        .getChannelBuffer(), false);
                 }
                 attribute = diskAttribute;
             }
diff --git a/src/main/java/org/jboss/netty/handler/codec/http/MixedFileUpload.java b/src/main/java/org/jboss/netty/handler/codec/http/MixedFileUpload.java
index 15d44119182..19ca643627d 100644
--- a/src/main/java/org/jboss/netty/handler/codec/http/MixedFileUpload.java
+++ b/src/main/java/org/jboss/netty/handler/codec/http/MixedFileUpload.java
@@ -57,7 +57,7 @@ public void addContent(ChannelBuffer buffer, boolean last)
                         definedSize);
                 if (((MemoryFileUpload) fileUpload).getChannelBuffer() != null) {
                     diskFileUpload.addContent(((MemoryFileUpload) fileUpload)
-                        .getChannelBuffer(), true);
+                        .getChannelBuffer(), false);
                 }
                 fileUpload = diskFileUpload;
             }


Test patch: