Fix maximum seek limit RecyclerBytesStreamOutput

[original-brownbear]

We must only seek to max_capacity - 1 max, that's the highest allowed index. If we seek to max capacity, we were creating a situation where current capacity became negative because we got to pages * page_size > Integer.MAX_VALUE.

[elasticsearchmachine]

Hi @original-brownbear, I've created a changelog YAML for you.

[elasticsearchmachine]

Pinging @elastic/es-distributed (Team:Distributed)

[kingherc]

Hi @original-brownbear , this fixes #95096 ? Maybe you can edit the PR/commit description for that. And would it be worth adding the relevant backport labels?

[original-brownbear]

@kingherc no but I just linked the relevant fix in that issue and closed it.

[kingherc]

because we got to pages * page_size > Integer.MAX_VALUE.

Where is that happening? I only see

public long position() { return ((long) pageSize * pageIndex) + currentPageOffset; } 

which is appropriately marked as long.

[DaveCTurner]

This seems a little odd to me. We certainly can't write anything after seeking to position Integer.MAX_VALUE - (Integer.MAX_VALUE % pageSize) but it seems like it should be legal to seek to that position. Otherwise if you write a byte at the end of that last page, then seek away, then you can't seek back to that spot again so bytes() will always yield a truncated value.

[original-brownbear]

@DaveCTurner you're right, we need to special case that seek ... Annoying, let me try :)

[DaveCTurner]

I think it's ok for ensureCapacityFromPosition to leave currentPageOffset pointing to the end of the current page. Maybe we only need to reset currentPageOffset in ensureCapacity (if bytesNeeded > 0)?

[original-brownbear]

@DaveCTurner

Maybe we only need to reset currentPageOffset in ensureCapacity (if bytesNeeded > 0)?

yea that's pretty much what it comes down to, except that we need to make sure to still do it if we already have capacity available from a previous seek (that was the other bug).
I solved this now by going back to the old logic and having a special case in seek for the case of falling on the page boundary. With that it all works out clean it seems.

[DaveCTurner]

How about this instead? (as a patch on f414d31)

diff --git a/server/src/main/java/org/elasticsearch/common/io/stream/RecyclerBytesStreamOutput.java b/server/src/main/java/org/elasticsearch/common/io/stream/RecyclerBytesStreamOutput.java index b6a5cbe897c..f1986185720 100644 --- a/server/src/main/java/org/elasticsearch/common/io/stream/RecyclerBytesStreamOutput.java +++ b/server/src/main/java/org/elasticsearch/common/io/stream/RecyclerBytesStreamOutput.java @@ -101,23 +101,25 @@ public class RecyclerBytesStreamOutput extends BytesStream implements Releasable @Override public void writeInt(int i) throws IOException { - if (4 > (pageSize - currentPageOffset)) { + ensureCapacity(Integer.BYTES); + if (Integer.BYTES > (pageSize - currentPageOffset)) { super.writeInt(i); } else { BytesRef currentPage = pages.get(pageIndex).v(); VH_BE_INT.set(currentPage.bytes, currentPage.offset + currentPageOffset, i); - currentPageOffset += 4; + currentPageOffset += Integer.BYTES; } } @Override public void writeLong(long i) throws IOException { - if (8 > (pageSize - currentPageOffset)) { + ensureCapacity(Long.BYTES); + if (Long.BYTES > (pageSize - currentPageOffset)) { super.writeLong(i); } else { BytesRef currentPage = pages.get(pageIndex).v(); VH_BE_LONG.set(currentPage.bytes, currentPage.offset + currentPageOffset, i); - currentPageOffset += 8; + currentPageOffset += Long.BYTES; } } @@ -151,15 +153,8 @@ public class RecyclerBytesStreamOutput extends BytesStream implements Releasable public void seek(long position) { ensureCapacityFromPosition(position); - int offsetInPage = (int) (position % pageSize); - int pageIndex = (int) position / pageSize; - if (offsetInPage == 0) { - this.pageIndex = pageIndex - 1; - this.currentPageOffset = pageSize; - } else { - this.pageIndex = pageIndex; - this.currentPageOffset = offsetInPage; - } + this.pageIndex = (int) position / pageSize; + this.currentPageOffset = (int) position % pageSize; } public void skip(int length) { @@ -221,6 +216,12 @@ public class RecyclerBytesStreamOutput extends BytesStream implements Releasable private void ensureCapacity(int bytesNeeded) { if (bytesNeeded > pageSize - currentPageOffset) { ensureCapacityFromPosition(position() + bytesNeeded); + + // We are at the end of the current page, increment page index + if (currentPageOffset == pageSize) { + pageIndex++; + currentPageOffset = 0; + } } } @@ -236,10 +237,5 @@ public class RecyclerBytesStreamOutput extends BytesStream implements Releasable pages.add(newPage); currentCapacity += pageSize; } - // We are at the end of the current page, increment page index - if (currentPageOffset == pageSize) { - pageIndex++; - currentPageOffset = 0; - } } }

[DaveCTurner]

oh wait that has the original bug again :/

[original-brownbear]

That's going if we add the capacity check to the common fast path I think, I'd rather not do that I think.
Seek is super rare, writing primitives is very common and sometimes very hot so we should optimise that IMO.

[DaveCTurner]

I see, ok that makes sense then; could you add a comment about that performance consideration here?

[original-brownbear]

Comment in the primitive write methods? :)

[DaveCTurner]

IMO a comment makes more sense in seek() - that's where the complex stuff is happening that looks like it would be neater if moved onto the hot path. Possibly also ensureCapacityFromPosition()

[original-brownbear]

++ sure thing, added a comment that I hope helps in ea0cf27

[DaveCTurner]

LGTM thanks Armin

[original-brownbear]

Jenkins run elasticsearch-ci/part-3

[original-brownbear]

np + thanks David!