Fixes #9166 - Jetty 12: review/remove ByteBufferPool

[sbordet]

• Replaced usages of ByteBufferPool with RetainableByteBufferPool.
• Removed ByteBufferPool and related classes.
• Updated all methods such as getBuffer() to getByteBuffer() for consistency.
• Updated javadocs.

Signed-off-by: Simone Bordet [email protected]

[sbordet]

@gregw @lachlan-roberts @lorban these are the major files to review:

oej.client.ContentDecoder
oej.fcgi.generator.*
oej.http.GZIPContentDecoder
oej.io.RetainableByteBuffer
oej.io.RetainableByteBufferPool
oej.server bytebufferpool-quadratic.mod & jetty-bytebufferpool-quadratic.xml
oej.server.GzipRequest
oej.server.AbstractConnector
oej.server.Server
oej.websocket.core.Parser
oej.websocket.core.PerMessageDeflateExtension

Note in particular that I changed the use of Server.getBean(...) for the Scheduler and the RetainableByteBufferPool.

Also changed the ArrayRetainableByteBufferPool.[Exponential|Logarithmic] to Quadratic.

One thing open for discussion is whether the Jetty module bytebufferpool.mod should be renamed to the mouthful retainablebytebufferpool.mod, or perhaps just bufferpool.mod.

[lachlan-roberts]

Is this a leak tracking RetainableByteBufferPool implementation? why delete TestableLeakTrackingBufferPool then.

[sbordet]

Because it was using the other pool class.

[lachlan-roberts]

But why not just update TestableLeakTrackingBufferPool to be a RetainableByteBufferPool then other tests can potentially use it.

[lachlan-roberts]

Why have these tests been commented out but you have a different change for the same ee9 test?

[sbordet]

Good catch, restore here too.

[gregw]

Wouldn't it be cool if code like this:

        RetainableByteBuffer retainableByteBuffer = byteBufferPool.acquire(getBufferSize(), isUseDirectByteBuffers());
        ByteBuffer byteBuffer = retainableByteBuffer.getByteBuffer();

        int read;
        try
        {
            BufferUtil.clearToFill(byteBuffer);
            read = read(channel, byteBuffer);
            BufferUtil.flipToFlush(byteBuffer, 0);
        }
        catch (Throwable x)
        {
            retainableByteBuffer.release();
            return failure(x);
        }

could become

        RetainableByteBuffer retainableByteBuffer = byteBufferPool.acquire(getBufferSize(), isUseDirectByteBuffers());

        int read;
        try
        {
            read = retainableByteBuffer.fill(channel);
        }
        catch (Throwable x)
        {
            retainableByteBuffer.release();
            return failure(x);
        }

The RetainableByteBuffer could then be renamed Buffer and have methods like:

• Buffer.fill(Channel)
• Buffer.fill(InputStream)
• Buffer.fill(EndPoint)
• Buffer.fill(Buffer)
• Buffer.fill(ByteBuffer)
• Buffer.fill(byte[],int,int)

But that is a different PR :)

[gregw]

First pass.

Generally i think we need to review all our acquires and comment how they are released if it is not obvious.

[lorban]

I have found one last problem in fcgi, but we should review the places where Greg found that we may leak a buffer.
And we should document the retain/release doctrine somewhere.