OutOfDirectMemoryError for large uploads using HttpPostMultipartRequestDecoder

[danielflower]

Expected behavior

With a HttpDataFactory that has useDisk=true, I thought files of any size could potentially be uploaded.

Actual behavior

Example error from below unit test:

io.netty.util.internal.OutOfDirectMemoryError: failed to allocate 4194304 byte(s) of direct memory (used: 62914560, max: 64487424)
	at io.netty.util.internal.PlatformDependent.incrementMemoryCounter(PlatformDependent.java:775)
	at io.netty.util.internal.PlatformDependent.reallocateDirectNoCleaner(PlatformDependent.java:748)
	at io.netty.buffer.UnpooledUnsafeNoCleanerDirectByteBuf.reallocateDirect(UnpooledUnsafeNoCleanerDirectByteBuf.java:34)
	at io.netty.buffer.UnpooledByteBufAllocator$InstrumentedUnpooledUnsafeNoCleanerDirectByteBuf.reallocateDirect(UnpooledByteBufAllocator.java:194)
	at io.netty.buffer.UnpooledUnsafeNoCleanerDirectByteBuf.capacity(UnpooledUnsafeNoCleanerDirectByteBuf.java:52)
	at io.netty.buffer.AbstractByteBuf.ensureWritable0(AbstractByteBuf.java:307)
	at io.netty.buffer.AbstractByteBuf.ensureWritable(AbstractByteBuf.java:282)
	at io.netty.buffer.AbstractByteBuf.writeBytes(AbstractByteBuf.java:1105)
	at io.netty.buffer.AbstractByteBuf.writeBytes(AbstractByteBuf.java:1098)
	at io.netty.buffer.AbstractByteBuf.writeBytes(AbstractByteBuf.java:1089)
	at io.netty.handler.codec.http.multipart.HttpPostMultipartRequestDecoder.offer(HttpPostMultipartRequestDecoder.java:351)
	at NettyUploadTest.itCanProcessLargeFiles(NettyUploadTest.java:46)
// snip

Steps to reproduce

Set -Xmx64m and run below unit test.

Minimal yet complete reproducer code (or URL to code)

import io.netty.buffer.ByteBuf;
import io.netty.buffer.Unpooled;
import io.netty.handler.codec.http.*;
import io.netty.handler.codec.http.multipart.DefaultHttpDataFactory;
import io.netty.handler.codec.http.multipart.FileUpload;
import io.netty.handler.codec.http.multipart.HttpDataFactory;
import io.netty.handler.codec.http.multipart.HttpPostMultipartRequestDecoder;
import org.junit.Test;

import java.nio.charset.StandardCharsets;
import java.util.Arrays;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;

public class NettyUploadTest {

    @Test
    public void itCanProcessLargeFiles() throws Exception {

        int fileSize = 100_000_000; // set Xmx to a number lower than this and it crashes
        int bytesPerChunk = 1_000_000;

        String prefix = "--861fbeab-cd20-470c-9609-d40a0f704466\n" +
            "Content-Disposition: form-data; name=\"image\"; filename=\"guangzhou.jpeg\"\n" +
            "Content-Type: image/jpeg\n" +
            "Content-Length: " + fileSize + "\n" +
            "\n";

        String suffix = "\n" +
            "--861fbeab-cd20-470c-9609-d40a0f704466--\n";

        HttpRequest request = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/upload");
        request.headers().set("content-type", "multipart/form-data; boundary=861fbeab-cd20-470c-9609-d40a0f704466");
        request.headers().set("content-length", prefix.length() + fileSize + suffix.length());

        HttpDataFactory factory = new DefaultHttpDataFactory(true);
        HttpPostMultipartRequestDecoder decoder = new HttpPostMultipartRequestDecoder(factory, request);
        decoder.offer(new DefaultHttpContent(Unpooled.wrappedBuffer(prefix.getBytes(StandardCharsets.UTF_8))));

        byte[] body = new byte[bytesPerChunk];
        Arrays.fill(body, (byte)1);
        for (int i = 0; i < fileSize / bytesPerChunk; i++) {
            ByteBuf content = Unpooled.wrappedBuffer(body, 0, bytesPerChunk);
            decoder.offer(new DefaultHttpContent(content)); // **OutOfMemory here**
            content.release();
        }

        decoder.offer(new DefaultHttpContent(Unpooled.wrappedBuffer(suffix.getBytes(StandardCharsets.UTF_8))));
        decoder.offer(new DefaultLastHttpContent());
        FileUpload data = (FileUpload) decoder.getBodyHttpDatas().get(0);
        assertThat((int)data.length(), is(fileSize));
        assertThat(data.get().length, is(fileSize));

        factory.cleanAllHttpData();

    }

}

Netty version

Tested on 4.1.56 and 4.1.58.

JVM version (e.g. java -version)

jdk1.8.0_162 and 12

OS version (e.g. uname -a)

Windows 10

[franz1981]

@fredericBregier It could be related to #10623 given that right now file isn't written until a delimiter is found?

[normanmaurer]

I think so too... I think we should never the commit in question for now as while it fixes some "perf issues" when running in with paranoid leak detection I don't think it has any other benefits in real world use-cases. @fredericBregier WDYT ?

[fredericBregier]

Hi, the issue is partially related but yet an issue.

• the file is totally in the buffer while reading: previously it could be in memory but by chunk
• once the file is over, if the "disk" based HttpData is used, the buffer is written to a temprary file, therefore leaving the memory free
• Note that another bug was fixed in the same time since before buffers were free (discardReadBytes) wrongly

I agree that this should be changed to adapt the solution with the new way to find the delimiter. Perhaps this?
Currently:

• The content is added to the HttpData along slices (see

  netty/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java

  Line 1191 in 5c52291

  httpData.addContent(content, true);

  ).
• But only once all slices are found (

  netty/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java

  Line 1171 in 5c52291

  newOffset = findDelimiter(undecodedChunk, delimiter, lastDataPosition);

  )

If it is written (in Disk mode or Mixed mode and if size is greater than limit), the buffer might be cleared (free), so one could try to change this such as:

• each time a slice is found (even if the delimiter is not found), one could add the content to the HttpData, but carefully take into consideration that perhaps not all bufer shall be taken (the end of the buffer could be in the middle of the delimiter, to not take into account)
• therefore, try to release the memory (careful, since if Memory based, must not be released at all or bad content will occur) as in

  netty/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java

  Line 348 in 5c52291

  if (undecodedChunk.refCnt() == 1 && writable < toWrite && readPos + writable >= toWrite) {

  netty/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java

  Line 349 in 5c52291

  undecodedChunk.discardReadBytes();

  ) (we cannot simply reset the writerIndex and readerIndex to the beginning for this HttpData since the HttpData could be in Memory only)

Not sure it will work or easy to implement, but I think the idea is there...

[fredericBregier]

Hi all, I propose a fix for this (adding a test inspired from the one given, testing both in Memory and on Disk behaviors)

[danielflower]

Thanks to both of you. The new version is working well now.

The performance degradation with paranoid detection is very noticeable (e.g. a test with a lot of data moving was taking 1 second in 4.1.58 takes 30 seconds in 4.1.59). For a while I thought there was a performance issue in the new version before I remembered what Frederic said about leak detection performance.

So if anyone else is experiencing this and wondering what is going on, try disabling PARANOID for the slow tests.

[fredericBregier]

@danielflower or maybe wait for #11001 which fixes also this PARANOID issue, and improves without this level also performances (about 4 times)... ;-)

[chrisvest]

The #11001 PR is merged now.