Netty5.x Multipart Codec for Netty Contrib

[pderop]

Motivation

This PR is an attempt to port the current Netty 4.1.80.Final-SNAPSHOT Multipart codec to the Netty 5.0.0-Alpha5 version.

Modifications

Mainly, bellow are the proposed changes that have been done (to be discussed :-)):

InterfaceHttpData.java

Before this interface was extending ReferenceCounted with the following methods:

    Int refCnt();
    ReferenceCounted retain();
    ReferenceCounted retain(int increment);
    ReferenceCounted touch();
    ReferenceCounted touch(Object hint);
    boolean release();
    boolean release(int decrement);

Now, the interface is now extending Resource, and the above methods are replaced by:

    Send<T> send();
    void close();
    boolean isAccessible();
    touch(Object hint);

So, we now have:

public interface InterfaceHttpData extends Comparable<InterfaceHttpData>, Resource<HttpData> {
    enum HttpDataType {
        Attribute, FileUpload, InternalAttribute
    }
    String getName();
    HttpDataType getHttpDataType();
}

HttpData.java

The HttpData interface does not extend the ByteBufHolder anymore, so, the following methods:

    ByteBuf content();
    ByteBufHolder copy();
    ByteBufHolder duplicate();
    ByteBufHolder retainedDuplicate();
    ByteBufHolder replace(ByteBuf content);
    ByteBufHolder retain();
    ByteBufHolder retain(int increment);
    ByteBufHolder touch();
    ByteBufHolder touch(Object hint);

are replaced by:

    Buffer content();
    HttpData copy();
    HttpData replace(Buffer content);

AbstractHttpData.java

The AbstractHttpData does not extend the AbstractReferenceCounted anymore. It instead extends ResourceSupport<HttpData, AbstractHttpData> in order to make it easier to implement the Resource interface. A Drop static inner class is now used to ensure that the HttpData.delete() method is invoked when the resource is closed.

The old AbstractReferenceCounted methods have been removed:

    public ByteBuf content();
    protected void deallocate();
    public HttpData retain();
    public HttpData retain(int increment);
    public abstract HttpData touch();
    public abstract HttpData touch(Object hint);

The content can now be obtained using content() method which is now returning a Buffer:

Buffer content();

HttpPostMultipartRequestDecoder.java

Before, the offer(HttpContent content) method was doing this check in order to possibly discard undecoded chunks:

        if (undecodedChunk != null && undecodedChunk.writerIndex() > discardThreshold) {
            if (undecodedChunk.refCnt() == 1) {
                // It's safe to call discardBytes() as we are the only owner of the buffer.
                undecodedChunk.discardReadBytes();
            } else {
                // There seems to be multiple references of the buffer. Let's copy the data and release the buffer to
                // ensure we can give back memory to the system.
                ByteBuf buffer = undecodedChunk.alloc().buffer(undecodedChunk.readableBytes());
                buffer.writeBytes(undecodedChunk);
                undecodedChunk.release();
                undecodedChunk = buffer;
            }
        }

now it is assumed that the content ownership is transferred to the MP codec, so the above code has been replaced by:

        if (undecodedChunk != null && undecodedChunk.writerOffset() > discardThreshold) {
            undecodedChunk.compact();
        }

Note: it may make sense to change the offer(HttpContent content) signature by offer(Send<HttpContent> content), in order to enforce the fact that the ownership is really transferred to the API. See below TODO section.

HttpPostStandardRequestDecoder.java:

The parseBodyAttributes method was using old ByteBuf.array() method in order to optimize parsing of body attributes, like this:

    private void parseBodyAttributes() {
        if (undecodedChunk == null) {
            return;
        }
        if (!undecodedChunk.hasArray()) {
            parseBodyAttributesStandard();
            return;
        }
        SeekAheadOptimize sao = new SeekAheadOptimize(undecodedChunk);
        ...
        // use the sao to optimizing parsing of body attribytes using undecodecChunk.array()

So, in the porting, the optimization has been removed because the Buffer API does not provide the byte array anymore,
and only the parseBodyAttributesStandard() has been kept in the new version:

    private void parseBodyAttributes() {
        if (undecodedChunk == null) {
            return;
        }
        parseBodyAttributesStandard();
    }

However, we can probably optimize the parseBodyAttributesStandard method with Cursors.

Result

The tests are passing, the new multipart codec is now based on netty5 buffers, and does not depend anymore on ByteBuf API.

Current status and improvements

Old multipart PRs from 4.1.80.Final-SNAPSHOT are merged, but there is still room for enhancements. I have created other PRs which can be addressed separately:

• Use cursors more appropriately Use cursors more appropriately  #2
• Enforce buffer ownership using Send Enforce buffer ownership using Send #3
• HttpPostRequestDecoder as a Resource HttpPostRequestDecoder as a Resource  #4
• AbstractMemoryHttpData performance issue when using composite buffer AbstractMemoryHttpData performance issue when using composite buffer #6

[pderop]

@normanmaurer , @chrisvest ,

Hi,

Could you please take a look at this PR ?

This PR is now functional, where the old benchmark and the old examples from 4.1.x are also ported. There are still rooms for enhancements, but I have created separate PRs which can be addressed separately (later):

• Use cursors more appropriately  #2
• Enforce buffer ownership using Send #3
• HttpPostRequestDecoder as a Resource  #4

thank you.

[pderop]

Merged the following PRs from 4.1 branch:

• Check and skip if post body parameter name and value are empty netty/netty#12427
• Track independent refCnt in MixedAttribute and MixedFileUpload netty/netty#12650
• Handle content-type optional parameters other than charset (netty#12663) netty/netty#12664

[pderop]

@chrisvest , @normanmaurer;

Could one of you kindly review this PR please? Indeed, we have another PR in Reactor-Netty that is pending and it depends on this PR in order reintegrate multipart using Netty5.

This PR is proposing a variant of the old API, but I'll be happy to finalise it in case you think there are problems with the proposed API changes. Now, there is still room for enhancements, but I have created separate PRs which can be addressed separately (later):

• Use cursors more appropriately Use cursors more appropriately  #2
• Enforce buffer ownership using Send Enforce buffer ownership using Send #3
• HttpPostRequestDecoder as a Resource HttpPostRequestDecoder as a Resource  #4
• AbstractMemoryHttpData performance issue when using composite buffer AbstractMemoryHttpData performance issue when using composite buffer #6

thanks !

[chrisvest]

@pderop I don't think we (Norman and I) have the bandwidth to help out with http multipart. I think you have the ability to merge it yourself, right? You can of course ask others in the community if you want more eyes on it.

[pderop]

@chrisvest ,

Ok, then I will see if someone else from the community would like to review this PR before I merge it.

[pderop]

The PR has been integrated in reactor-netty draft PR (reactor/reactor-netty#2327), and from discord, there is a feedback regarding scalability of instanceof checks (I have created the #10 PR in order to investigate this a bit later).

So, In the meantime, let's merge this PR into main.