Assert Not Escaping byte[] from Pooled ByteBuf

[original-brownbear]

• We shouldn't just be using the byte array from poole buffers here. Doing so relies on the fact that we will never use the BytesRef after releasing the wrapped ByteBuf
• Related to the investigation in [DOCS] Remove heading offsets for REST APIs #44568
  • If we're afraid of not copying the ByteBuf there because the derived BytesRef from it would contain garbage if they were to live beyond releasing the ByteBuf, shouldn't we then prevent this (using the array from a pooled buffer to back a BytesRef from ever happening?
  • This PR adds the necessary assertion for that (shamelessly stolen from Reduce garbage for requests with unpooled buffers #32228. I didn't want to actually prevent this (escaping the array from a pooled buffer) from happening yet (technically it would be fine unless we release the buffer too early) because it could have very serious performance implications in production in some cases but figured we should get some visibility on this

[elasticmachine]

Pinging @elastic/es-distributed

[original-brownbear]

Jenkins run elasticsearch-ci/packaging-sample

[original-brownbear]

Jenkins run elasticsearch-ci/1

[andrershov]

@original-brownbear I want to better understand why you propose to add assertion here.
Do I understand correctly that currently we never use pooled byte buffers, that are wrapped with ByteBufferByteReference and you want to prevent this in the future?

[original-brownbear]

@andrershov

Do I understand correctly that currently we never use pooled byte buffers, that are wrapped with ByteBufferByteReference and you want to prevent this in the future?

We do use ByteBufferBytesReference based on pooled ByteBuf in a few places but we don't use them to create BytesRef by invoking org.elasticsearch.http.nio.ByteBufUtils.ByteBufBytesReference#toBytesRef.

I'd like to prevent us from doing that in the future because it could lead to some severe and hard to track down bugs. Those would happen if we would create the BytesRef from the pooled byte[] in a ByteBuf that is pooled but still returns it's underlying array (PooledHeapByteBuf and CompositeByteBuf that consist in only a single PooledHeapByteBuf). If we were to use the BytesRef from such a ByteBuf past the lifetime of the ByteBuf the contents of the array could change while we process it elsewhere.

[andrershov]

If you have created ByteBufBytesReference from the pooled ByteBuf and you have released pooled ByteBuf, you can no longer work with ByteBufBytesReference methods, such as get, indexOf, etc.
So you kind of expect that you should never release ByteBuf until ByteBufBytesReference goes out of scope.
If you call toBytesRef method you also expect that you should not release ByteBuf until returned BytesRef goes out of scope.
Probably we should just add this kind of docs to ByteBufBytesReference class.
I mean the assertion that you propose to add does not make ByteBufBytesReference class completely safe, but additionally, it restricts this class unsafe usage.
If documentation is not enough from your point of view, I suggest to 2 separate classes SafeByteBufBytesReference and UnsafeByteBufBytesReference, the first one will check that passed in ByteBuf is unpooled during construction.

[original-brownbear]

@andrershov

I mean the assertion that you propose to add does not make ByteBufBytesReference class completely safe

I would argue in tests it will recognize any unsafe usage?

If documentation is not enough from your point of view, I suggest to 2 separate classes SafeByteBufBytesReference and UnsafeByteBufBytesReference, the first one will check that passed in ByteBuf is unpooled during construction.

What does that get us? We are creating these instances on the network layer and passing them down to transport handlers. We need to ensure that transport handlers don't do the wrong thing on pooled buffers and start to create unsafe BytesRef from the BytesReference (or a slice of it) way down the call stack. Checking this on create time doesn't really help with that?

[andrershov]

LGTM

[original-brownbear]

#44564 makes this assertion wrong, we are now conciously escaping these bytes at times.