Change read-only behavior of composite buffers for netty 5

[yawkat]

Prototype. @chrisvest thoughts?

Motivation:

A common use case for composite buffers is to aggregate data and then send it on, without copying. One example is HttpObjectAggregator, which combines multiple HttpContent objects into a single composite buffer, and then sends it on as a FullHttpMessage. In these use cases, the composite buffer is essentially treated as an equivalent but copy-less alternative to repeated writeBytes calls to a normal buffer.

When it comes to read-only buffers however, the composite buffer has a worse interaction with the buffers "written to it" than a normal buffer has. A composite buffer "guesses" its RO state from its first component, and any further components have to match that RO state.

For HttpObjectAggregator, this essentially means that you cannot send RO HttpContent objects, because netty generates RW buffers in most places, and RW and RO cannot be mixed. This is despite the fact that downstream consumers of the FullHttpMessage are unlikely to actually write to the HttpContent, and would be fine with an RO composite.

In netty 4, this restriction on mixing did not exist, and it would only blow up when a write attempt was made.

A workaround in netty 5 is to explicitly make any inputs read-only on extendWith. However since this aggregation pattern is so common, it is easy to miss this step and create code that is incompatible with RO buffers when it doesn't need to be, as is the case for HttpObjectAggregator.

Modification:

This PR explores a different approach. Composites do not infer their RO state from the first component anymore, and are instead constructed explicitly in RO or RW state. RO composites can now accept RW buffers, and make them RO on extend. RW composites retain the old behavior and will fail when an RO buffer is added. The compose method creates an RW composite by default (will now fail if the input or first extend call is RO), and there is a new composeReadOnly method to create an RO composite.

Result:

Classes like HttpObjectAggregator could now use composeReadOnly and become compatible with both RO and RW inputs without further changes. This also removes some of the "magic" of the automatic RO state detection. However it still requires a change in all the classes that use this pattern, and for more generic use cases (CoalescingBufferQueue), removes the ability to create RW composites when the developer knows all inputs are RW.

Further work: (edit: implemented, see below)

A further change could be to allow extending RW composites with RO buffers, but making the composite (and its earlier components) RO when this happens. This would allow us to remove the compositeReadOnly methods again. For the aggregation use case, it would be very useful, because RO buffers can be used but if the developers control all inputs and ensure they're RW, they can still rely on the output being RW. It would also make the transition from netty 4 easier.

The big downside of allowing this is that it moves potential errors from the site that causes them. It might not be easy to tell which code adds an RO buffer and "pollutes" the composite causing it to become RO, and then causing code further downstream to break.

In my opinion however, since this aggregation use case is quite common, this would be a worthwhile tradeoff.

[chrisvest]

@yawkat I like where this is going. It also seems to close the loop-hole where if a composite buffer ends up with zero components, it can forget if it's read-only or not.

[yawkat]

What do you think of the automatic demotion from RW to RO idea? As it stands now, this PR doesn't fully address my original issue, since it still requires us to move all relevant users to the new compositeReadOnly.

[chrisvest]

I think that's fine because ownership passes to the composite, and the composite is RO. You won't be messing with anyone else's buffer.

[yawkat]

I've added a new commit with the change.

• The constructor will again deduce the RO state from the input. However it's not the first buffer that counts. Instead, if any buffer is RO, all are made RO.
• There is a new "copy constructor" that takes an RO flag and checks whether the components match it. This also handles the empty split case.
• extendWith now "demotes" the composite from RW to RO, if the new component is RO.
• composeReadOnly gone again. Now that passing mixed buffers to compose is allowed, compose(...).makeReadOnly() is equivalent.

[chrisvest]

The big downside of allowing this is that it moves potential errors from the site that causes them. It might not be easy to tell which code adds an RO buffer and "pollutes" the composite causing it to become RO, and then causing code further downstream to break.

I guess this didn't really register with me the first time around; that you were talking about "future work" and not the PR as it stood at the time. I thought RW->RO demotion would only be for component buffers added to a RO composite buffer.
I find it kind of weird that extending a composite with an "arbitrary" buffer could cause the composite to change writability. Even if it require use cases like CoalescingBufferQueue to be explicitly configured for RW or RO, I still prefer the first idea.

[yawkat]

Yea, the extend behavior is required to stay consistent (compose(x, y) should be equivalent to compose(x).extendWith(y)), but it's definitely weird. In netty 4, extending with a RO buffer makes the composite "partially RO" iirc, which is conceptually similar (a partially RO composite is for many purposes fully RO) but more in line with user expectations.

What I fear from the initial changeset, without automatic RW->RO demotion, is a "fractured ecosystem": All places right now use read-write compose() (and people will keep using it by default). We would need to migrate many of them to composeReadOnly() in order to accept RO buffers. Those produced RO composite buffers would then be incompatible with those users that still use compose() (as is the case right now, HttpObjectAggregator won't eat RO buffers). And if you run into this incompatibility as a user, there's little you can do because it needs a change in a handler you may not control.

---

A third option I just thought of would be to take the initial PR, but rename the construction methods. compose would become composeWritable, while composeReadOnly would become compose. This would be a big change of course because it would prevent writing to "default" composites altogether, but in those cases where a composite isn't written to directly and only extend is used (are those most cases?), we would now be "compatible by default" with RO input buffers–including the RO composites produced by other handlers. Places that do write to the composite would notice so easily and could move to composeWritable, and guard their extend inputs to handle RO buffers properly.

WDYT of this option?

[yawkat]

this pr should also wait until people are back from vacation. maybe someone else has a good idea how to resolve this.