Skip to content

Commit

Permalink
90: BufferedData slices are still broken (#91)
Browse files Browse the repository at this point in the history 
Fixes: #90
Reviewed-by: Ivan Malygin <[email protected]>, Richard Bair <[email protected]>
Signed-off-by: Artem Ananev <[email protected]>
  • Loading branch information
@artemananiev
artemananiev authored Aug 29, 2023
1 parent 31020df commit ba9237b
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 31 deletions.
2 changes: 1 addition & 1 deletion pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/ReadableSequentialData.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ default long readBytes(@NonNull final BufferedData dst) {
/**
* Read {@code length} bytes from this sequence, returning them as a {@link Bytes} buffer of
* the read data. The returned bytes will be immutable. The {@link #position()} of this sequence will be
* incremented by {@code maxLength} bytes.
* incremented by {@code length} bytes.
*
* <p>Bytes are read from the sequence one at a time. If there are not {@code length} bytes remaining in this
* sequence, then a {@link BufferUnderflowException} will be thrown. The {@link #position()} will be
Expand Down
43 changes: 23 additions & 20 deletions pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/buffer/BufferedData.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package com.hedera.pbj.runtime.io.buffer;

import static java.nio.ByteOrder.BIG_ENDIAN;

import com.hedera.pbj.runtime.io.DataAccessException;
import com.hedera.pbj.runtime.io.ReadableSequentialData;
import com.hedera.pbj.runtime.io.WritableSequentialData;
Expand All @@ -11,7 +13,6 @@
import java.nio.ByteBuffer;
import java.nio.ByteOrder;
import java.util.Objects;
import static java.nio.ByteOrder.BIG_ENDIAN;

/**
* A buffer backed by a {@link ByteBuffer} that is a {@link BufferedSequentialData} (and therefore contains
Expand Down Expand Up @@ -183,7 +184,7 @@ public String toString() {
for (int i = 0; i < buffer.limit(); i++) {
int v = buffer.get(i) & 0xFF;
sb.append(v);
if (i < (buffer.limit()-1)) sb.append(',');
if (i < (buffer.limit() - 1)) sb.append(',');
}
sb.append(']');
return sb.toString();
Expand Down Expand Up @@ -253,7 +254,7 @@ public long limit() {
@Override
public void limit(final long limit) {
final var lim = Math.min(capacity(), Math.max(limit, position()));
buffer.limit((int)lim);
buffer.limit((int) lim);
}

/**
Expand Down Expand Up @@ -299,7 +300,7 @@ public void position(final long position) {

/**
* Set the limit to current position and position to origin. This is useful when you have just finished writing
* into a buffer and want to flip it ready to read back from.
* into a buffer and want to flip it ready to read back from, or vice versa.
*/
@Override
public void flip() {
Expand Down Expand Up @@ -508,9 +509,9 @@ public long readBytes(@NonNull final ByteBuffer dst) {
final var len = Math.toIntExact(Math.min(dst.remaining(), remaining()));
if (len == 0) return 0;

final int dtsPos = dst.position();
dst.put(dtsPos, buffer, Math.toIntExact(position()), len);
dst.position(dtsPos + len);
final int dstPos = dst.position();
dst.put(dstPos, buffer, Math.toIntExact(position()), len);
dst.position(dstPos + len);
position(position() + len);
return len;
}
Expand Down Expand Up @@ -664,20 +665,21 @@ public double readDouble(@NonNull final ByteOrder byteOrder) {
public int readVarInt(final boolean zigZag) {
if (direct) return ReadableSequentialData.super.readVarInt(zigZag);

int tempPos = buffer.position() + buffer.arrayOffset();
final int arrayOffset = buffer.arrayOffset();
int tempPos = buffer.position() + arrayOffset;
final byte[] buff = buffer.array();
int lastPos = buffer.limit() + buffer.arrayOffset();
int lastPos = buffer.limit() + arrayOffset;

if (lastPos == tempPos) {
throw new BufferUnderflowException();
}

int x;
if ((x = buff[tempPos++]) >= 0) {
buffer.position(tempPos);
buffer.position(tempPos - arrayOffset);
return zigZag ? (x >>> 1) ^ -(x & 1) : x;
} else if (lastPos - tempPos < 9) {
return (int)readVarIntLongSlow(zigZag);
return (int) readVarIntLongSlow(zigZag);
} else if ((x ^= (buff[tempPos++] << 7)) < 0) {
x ^= (~0 << 7);
} else if ((x ^= (buff[tempPos++] << 14)) >= 0) {
Expand All @@ -694,10 +696,10 @@ public int readVarInt(final boolean zigZag) {
&& buff[tempPos++] < 0
&& buff[tempPos++] < 0
&& buff[tempPos++] < 0) {
return (int)readVarIntLongSlow(zigZag);
return (int) readVarIntLongSlow(zigZag);
}
}
buffer.position(tempPos - buffer.arrayOffset());
buffer.position(tempPos - arrayOffset);
return zigZag ? (x >>> 1) ^ -(x & 1) : x;
}

Expand All @@ -708,9 +710,10 @@ public int readVarInt(final boolean zigZag) {
public long readVarLong(boolean zigZag) {
if (direct) return ReadableSequentialData.super.readVarLong(zigZag);

int tempPos = buffer.position() + buffer.arrayOffset();
final int arrayOffset = buffer.arrayOffset();
int tempPos = buffer.position() + arrayOffset;
final byte[] buff = buffer.array();
int lastPos = buffer.limit() + buffer.arrayOffset();
int lastPos = buffer.limit() + arrayOffset;

if (lastPos == tempPos) {
throw new BufferUnderflowException();
Expand All @@ -719,7 +722,7 @@ public long readVarLong(boolean zigZag) {
long x;
int y;
if ((y = buff[tempPos++]) >= 0) {
buffer.position(tempPos);
buffer.position(tempPos - arrayOffset);
return zigZag ? (y >>> 1) ^ -(y & 1) : y;
} else if (lastPos - tempPos < 9) {
return readVarIntLongSlow(zigZag);
Expand Down Expand Up @@ -761,7 +764,7 @@ public long readVarLong(boolean zigZag) {
}
}
}
buffer.position(tempPos);
buffer.position(tempPos - arrayOffset);
return zigZag ? (x >>> 1) ^ -(x & 1) : x;
}

Expand All @@ -780,7 +783,7 @@ public void writeByte(final byte b) {
* {@inheritDoc}
*/
public void writeUnsignedByte(final int b) {
buffer.put((byte)b);
buffer.put((byte) b);
}

/**
Expand Down Expand Up @@ -913,7 +916,7 @@ public void writeInt(final int value, @NonNull final ByteOrder byteOrder) {
@Override
public void writeUnsignedInt(final long value) {
buffer.order(BIG_ENDIAN);
buffer.putInt((int)value);
buffer.putInt((int) value);
}

/**
Expand All @@ -922,7 +925,7 @@ public void writeUnsignedInt(final long value) {
@Override
public void writeUnsignedInt(final long value, @NonNull final ByteOrder byteOrder) {
buffer.order(byteOrder);
buffer.putInt((int)value);
buffer.putInt((int) value);
}

/**
Expand Down
3 changes: 2 additions & 1 deletion ...re/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/buffer/BufferedSequentialData.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ public interface BufferedSequentialData extends SequentialData, RandomAccessData

/**
* Set the {@link #limit()} to the current {@link #position()} and the {@link #position()} to the origin. This is
* useful when you have just finished writing into a buffer and want to flip it to be ready to read back from.
* useful when you have just finished writing into a buffer and want to flip it to be ready to read back from, or
* vice versa.
*/
void flip();

Expand Down
85 changes: 76 additions & 9 deletions pbj-core/pbj-runtime/src/test/java/com/hedera/pbj/runtime/io/buffer/BufferedDataTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package com.hedera.pbj.runtime.io.buffer;

import static org.junit.jupiter.api.Assertions.assertEquals;

import com.hedera.pbj.runtime.io.ReadableSequentialData;
import com.hedera.pbj.runtime.io.ReadableTestBase;
import com.hedera.pbj.runtime.io.WritableSequentialData;
Expand All @@ -8,7 +10,8 @@
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.assertEquals;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

final class BufferedDataTest {
// FUTURE Test that "view" shows the updated data when it is changed on the fly
Expand All @@ -17,16 +20,80 @@ final class BufferedDataTest {
@Test
@DisplayName("toString() is safe")
void toStringIsSafe() {
final var buf = BufferedData.wrap(new byte[]{1, 2, 3, 4, 5, 6, 7, 8, 9, 10});
final var buf = BufferedData.wrap(new byte[] {1, 2, 3, 4, 5, 6, 7, 8, 9, 10});
buf.skip(5);
buf.limit(10);

@SuppressWarnings("unused")
final var ignored = buf.toString();

assertEquals(5, buf.position());
assertEquals(10, buf.limit());
}

@ParameterizedTest
@ValueSource(ints = {0, 1, 2, 4, 7, 8, 15, 16, 31, 32, 33, 127, 128, 512, 1000, 1024, 4000,
16384, 65535, 65536, 65537, 0xFFFFFF, 0x1000000, 0x1000001, 0x7FFFFFFF,
-1, -7, -8, -9, -127, -128, -129, -65535, -65536, -0xFFFFFF, -0x1000000, -0x1000001, -0x80000000})
@DisplayName("readVarInt() works with views")
void sliceThenReadVarInt(final int num) {
final var buf = BufferedData.allocate(100);
buf.writeVarInt(num, false);
final long afterFirstIntPos = buf.position();
buf.writeVarInt(num + 1, false);
final long afterSecondIntPos = buf.position();

final var slicedBuf = buf.slice(afterFirstIntPos, afterSecondIntPos - afterFirstIntPos);
final int readback = slicedBuf.readVarInt(false);
assertEquals(num + 1, readback);
assertEquals(afterSecondIntPos - afterFirstIntPos, slicedBuf.position());
}

@ParameterizedTest
@ValueSource(longs = {0, 1, 7, 8, 9, 127, 128, 129, 1023, 1024, 1025, 65534, 65535, 65536,
0xFFFFFFFFL, 0x100000000L, 0x100000001L, 0xFFFFFFFFFFFFL, 0x1000000000000L, 0x1000000000001L,
-1, -7, -8, -9, -127, -128, -129, -65534, -65535, -65536, -0xFFFFFFFFL, -0x100000000L, -0x100000001L})
@DisplayName("readVarLong() works with views")
void sliceThenReadVarLong(final long num) {
final var buf = BufferedData.allocate(256);
buf.writeVarLong(num, false);
final long afterFirstIntPos = buf.position();
buf.writeVarLong(num + 1, false);
final long afterSecondIntPos = buf.position();

final var slicedBuf = buf.slice(afterFirstIntPos, afterSecondIntPos - afterFirstIntPos);
final long readback = slicedBuf.readVarLong(false);
assertEquals(num + 1, readback);
assertEquals(afterSecondIntPos - afterFirstIntPos, slicedBuf.position());
}

@Test
@DisplayName("readBytes() works with views")
void sliceThenReadBytes() {
final int SIZE = 100;
final var buf = BufferedData.allocate(SIZE);
for (int i = 0; i < SIZE; i++) {
buf.writeByte((byte) i);
}

final int START = 10;
final int LEN = 10;
buf.reset();
buf.skip(START);
final var bytes = buf.readBytes(LEN);
assertEquals(START + LEN, buf.position());
for (int i = START; i < START + LEN; i++) {
assertEquals((byte) i, bytes.getByte(i - START));
}

final var slicedBuf = buf.slice(START, LEN);
final var slicedBytes = slicedBuf.readBytes(LEN);
assertEquals(LEN, slicedBuf.position());
for (int i = 0; i < LEN; i++) {
assertEquals((byte) (i + START), slicedBytes.getByte(i));
}
}

@Nested
final class ReadableSequentialDataTest extends ReadableTestBase {

Expand All @@ -39,7 +106,7 @@ protected ReadableSequentialData emptySequence() {
@NonNull
@Override
protected ReadableSequentialData fullyUsedSequence() {
final var buf = BufferedData.wrap(new byte[]{1, 2, 3, 4, 5, 6, 7, 8, 9, 10});
final var buf = BufferedData.wrap(new byte[] {1, 2, 3, 4, 5, 6, 7, 8, 9, 10});
buf.skip(10);
return buf;
}
Expand All @@ -63,7 +130,7 @@ protected ReadableSequentialData emptySequence() {
@NonNull
@Override
protected ReadableSequentialData fullyUsedSequence() {
final var buf = BufferedData.wrap(new byte[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 });
final var buf = BufferedData.wrap(new byte[] {1, 2, 3, 4, 5, 6, 7, 8, 9, 10});
buf.skip(10);
return new RandomAccessSequenceAdapter(buf, 10);
}
Expand Down Expand Up @@ -105,15 +172,15 @@ final class ReadableSequentialDataViewTest extends ReadableTestBase {
@NonNull
@Override
protected ReadableSequentialData emptySequence() {
final var buf = BufferedData.wrap(new byte[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 });
final var buf = BufferedData.wrap(new byte[] {1, 2, 3, 4, 5, 6, 7, 8, 9, 10});
buf.position(7);
return buf.view(0);
}

@NonNull
@Override
protected ReadableSequentialData fullyUsedSequence() {
final var buf = BufferedData.wrap(new byte[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 });
final var buf = BufferedData.wrap(new byte[] {1, 2, 3, 4, 5, 6, 7, 8, 9, 10});
buf.position(2);
final var view = buf.view(5);
view.skip(5);
Expand Down Expand Up @@ -144,15 +211,15 @@ final class RandomAccessDataViewTest extends ReadableTestBase {
@NonNull
@Override
protected ReadableSequentialData emptySequence() {
final var buf = BufferedData.wrap(new byte[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 });
final var buf = BufferedData.wrap(new byte[] {1, 2, 3, 4, 5, 6, 7, 8, 9, 10});
buf.position(7);
return new RandomAccessSequenceAdapter(buf.view(0));
}

@NonNull
@Override
protected ReadableSequentialData fullyUsedSequence() {
final var buf = BufferedData.wrap(new byte[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 });
final var buf = BufferedData.wrap(new byte[] {1, 2, 3, 4, 5, 6, 7, 8, 9, 10});
buf.position(2);
final var view = buf.view(5);
view.skip(5);
Expand Down Expand Up @@ -191,7 +258,7 @@ protected WritableSequentialData sequence() {
@NonNull
@Override
protected WritableSequentialData eofSequence() {
final var buf = BufferedData.wrap(new byte[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 });
final var buf = BufferedData.wrap(new byte[] {1, 2, 3, 4, 5, 6, 7, 8, 9, 10});
buf.position(7);
return buf.view(0);
}
Expand Down

0 comments on commit ba9237b

Please sign in to comment.