Content.Chunk.isTerminal() cannot discriminate EOF from chunks containing a pooled empty buffer

[lorban]

Jetty version(s)
12.0.x

Description
Content.Chunk.isTerminal() has a contract that allows some ambiguity: it is not possible to distinguish a Content.Chunk that contains a pool-acquired zero-byte buffer and is last from Content.Chunk.EOF.

Since Content.Chunk.EOF.retain() throws UnsupportedOperationException, each time a new chunk is read and before it is wrapped, it must be checked to make sure it isn't EOF as retain() cannot be called on the latter.

We currently use isTerminal() to perform that check but since it cannot discriminate EOF from an empty-and-last chunk we either leak the chunk's buffer or double-release it.

The Content.Chunk creation, isTerminal() and Retainable contracts should be reworked to lift this ambiguity.

[lorban]

I can see two paths to solve this problem:

1. The loosening way. Change Content.Chunk.EOF so that it allows calling retain() and always call retain()/release() on all chunks.
2. The hardening way. Make sure we never create Content.Chunk instances that are empty-and-last and always use EOF in that case.

(1) requires a much looser description of the Retainable contract as each call to retain() doesn't need to be paired with a release() call before the latter returns true.

(2) requires a stricter Content.Chunk creation contract that specifies that passing empty buffer when last is true isn't legal, possibly with some code assertions.

Both options probably have an impact on Content.Chunk.EMPTY and Content.Chunk.Error, this needs to be investigated too.

[lorban]

Option (2) seems to be the preferred one as it clearly requires the least amount of rework: the server code already assumes this contract.

Required changes needed to adapt the client code are rather minimal.

[gregw]

Why does retain on EOF throw? Surely retain and release on a chunk with no buffers are just noops?

Note eitherway, i still like the PR to use EOF when possible

[lorban]

The main reason why it was decided that calling retain on EOF would throw is because it isn't possible to properly implement the retain/release contract w.r.t the boolean returned by release().

The root cause of this bug is that the current API doesn't allow implementing a noop retain/release (the Retainable javadoc is too strict) but it's not possible to discriminate chunks upon which we cannot call retain. This is why I listed the two options: either loosen the Retainable contract or harden Content.Chunk so that it doesn't allow creating instances upon which retain cannot be called, forcing the use of the EOF constant, and making isTerminal() reliably detect such case.

Most server-side usages have respected the implicit rule of do-not-create-empty-buffer-last-chunks-always-use-EOF-instead which is why I think making it explicit in the contract and adding some safety checks is the right way forward.

[gregw]

Wouldn't a noop implementation of Retainable be:

    class Noop implements Retainable
    {
        @Override
        public void retain()
        {
        }

        @Override
        public boolean release()
        {
            return true;
        }
    }

With that, we don't need to throw from EOF retain (as there is nothing to retain).

However, I still think it is worthwhile to use EOF where possible, so this PR is good regardless. So I think we should do both: loosen the Retainable contract so a noop impl is possible, but also harden our usage/utilities of/for Chunk so that EOF is used when possible.

[lorban]

@gregw I wasn't against that idea at first, quite the opposite. But @sbordet managed to convince me that there was not any good use-case for a noop impl as it always is a missed opportunity for using EOF AFAICT.

[gregw]

@gregw I wasn't against that idea at first, quite the opposite. But @sbordet managed to convince me that there was not any good use-case for a noop impl as it always is a missed opportunity for using EOF AFAICT.

@sbordet can you convince me too? This is not a show stopper for me, but I do think the code would be cleaner if we didn't have to worry about not being able to retain some types of chunks.

[sbordet]

Fixed by #9073 and #9159.