Fix offset in acquire, inside() method #498

nicktindall · 2023-03-21T07:33:56Z

Fixes #481

Hide whitespace when reviewing, one file had an extra indent through the whole file that I removed

nicktindall · 2023-03-21T08:15:01Z

src/main/java/net/openhft/chronicle/bytes/internal/ChunkedMappedBytes.java

-            else
-                acquireNextByteStore0(offset + adding - 1, false);
+        if (adding > 0 && !bytesStore.inside(offset, checkSize0(adding))) {
+            acquireNextByteStore0(offset, false);


The old else is the cause of #452, this bug is repeated throughout the bytes implementations. The basic error is

if (!bytesStore.inside(offset, length)) { bytesStore.acquire(offset + length); }

This works most of the time when you're writing forwards sequentially because if what you're writing won't fit in currentStore + offset, it'll usually fit in the store that gets acquired (nextStore) because that will start at currentStore + 1

The error shows up in the forward direction when you jump ahead a cycle (see what I added to the test)

e.g.

for a mapped file with chunksize 100, overlap 20

the sequential case

writeLong(50, some-long); // positioned in the first chunk writeLong(119, some-long); -> if (!bytesStore.inside(119, 8)) { // false bytesStore.acquire(127); // loads second store, which happens to contain 119->127, so OK

the non-sequential case (broken)

writeLong(50, some-long); // positioned in the first chunk writeLong(199, some-long); -> if (!bytesStore.inside(199, 8)) { // false bytesStore.acquire(207); // loads third store, which does not contain 199->207, so FAIL

the backwards traversal case (broken)

writeLong(150, some-long); // positioned in the second chunk writeLong(99, some-long); -> if (!bytesStore.inside(99, 8)) { // false bytesStore.acquire(107); // loads second store, which does not contain 99->107, so FAIL

nicktindall · 2023-03-21T08:15:51Z

src/main/java/net/openhft/chronicle/bytes/internal/ChunkedMappedBytes.java

-        if (!bytesStore.inside(offset, 1)) {
-            bytesStore = acquireNextByteStore0(offset + 1, false);
+        if (!bytesStore.inside(offset, 2)) {
+            bytesStore = acquireNextByteStore0(offset, false);


example of the bug

Agreed, I saw that on Monday.

nicktindall · 2023-03-21T08:16:04Z

src/main/java/net/openhft/chronicle/bytes/internal/ChunkedMappedBytes.java

-        if (!bytesStore.inside(offset, 3)) {
-            bytesStore = acquireNextByteStore0(offset + 3, false);
+        if (!bytesStore.inside(offset, 4)) {
+            bytesStore = acquireNextByteStore0(offset, false);


nicktindall · 2023-03-21T08:16:12Z

src/main/java/net/openhft/chronicle/bytes/internal/ChunkedMappedBytes.java

-        if (!bytesStore.inside(offset, 7)) {
-            bytesStore = acquireNextByteStore0(offset + 7, false);
+        if (!bytesStore.inside(offset, 8)) {
+            bytesStore = acquireNextByteStore0(offset, false);


nicktindall · 2023-03-21T08:16:28Z

src/main/java/net/openhft/chronicle/bytes/internal/ChunkedMappedBytes.java

-        if (!bytesStore.inside(readPosition, 3)) {
-            bytesStore = acquireNextByteStore0(readPosition + 3, true);
+        if (!bytesStore.inside(readPosition, 4)) {
+            bytesStore = acquireNextByteStore0(readPosition, true);


nicktindall · 2023-03-21T08:17:10Z

src/main/java/net/openhft/chronicle/bytes/internal/NoBytesStore.java

-    public boolean inside(@NonNegative long offset, @NonNegative long buffer) {
-        return false;
+    public boolean inside(@NonNegative long offset, @NonNegative long bufferSize) {
+        return offset == 0 && bufferSize == 0;


an array of zero length at position zero is inside the empty store

nicktindall · 2023-03-21T08:18:13Z

src/test/java/net/openhft/chronicle/bytes/MappedBytesEdgeTest.java

+                    // go back to the first store
+                    checkWritePosition(bytes, CHUNK_SIZE - 1, 0);
+                    // write to just before the start of the third bytes store (i.e. not in the 1st plus overlap or 2nd plus overlap)
+                    checkWritePosition(bytes, 3 * CHUNK_SIZE - size + 1, 2 * CHUNK_SIZE);


This reproduces the bug. If you pop the last commit (with the fix in it) off, you will see it blow.

nicktindall · 2023-03-21T08:19:27Z

src/main/java/net/openhft/chronicle/bytes/BytesStore.java

-     * as the 1st byte is written at offset, and the last at offset+n-1
-     * <p>
-     * Use this test to determine if an offset is considered safe to write to. Note that it checks we are
-     * inside the BytesStore limits *including* the overlap


I still feel strongly that we make this intuitive rather than writing a convoluted comment as to why it's not and how to use it "correctly".

JerryShea · 2023-03-28T05:37:56Z

src/main/java/net/openhft/chronicle/bytes/AbstractBytes.java

@@ -213,21 +212,21 @@ public long start() {
    @Override
    public boolean compareAndSwapInt(@NonNegative long offset, int expected, int value)
            throws BufferOverflowException, IllegalStateException {
-        writeCheckOffset(offset, 4);
+        writeCheckOffset(offset, Integer.BYTES);


…onstants, ensure overlapSize <= chunkSize, remove minimum overlapSize. Fixes #481

sonarcloud · 2023-03-30T06:56:37Z

Kudos, SonarCloud Quality Gate passed!

1 Bug
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

75.2% Coverage
2.6% Duplication

nicktindall force-pushed the temp/overlap-vs-chunk branch from 9db8446 to 4066cfd Compare March 21, 2023 08:10

nicktindall commented Mar 21, 2023

View reviewed changes

nicktindall requested a review from JerryShea March 21, 2023 08:20

nicktindall force-pushed the temp/overlap-vs-chunk branch 4 times, most recently from 10aedc9 to d47b069 Compare March 23, 2023 07:27

nicktindall changed the title ~~For discussion, not merging DO NOT MERGE~~ Fix offset in acquire, inside() method Mar 23, 2023

nicktindall force-pushed the temp/overlap-vs-chunk branch from d47b069 to d581211 Compare March 23, 2023 07:31

JerryShea reviewed Mar 28, 2023

View reviewed changes

JerryShea approved these changes Mar 29, 2023

View reviewed changes

nicktindall force-pushed the temp/overlap-vs-chunk branch 2 times, most recently from 7317da2 to 80530a3 Compare March 30, 2023 06:43

Fix up inside(...) and calls to it, replace some magic numbers with c…

f359b12

…onstants, ensure overlapSize <= chunkSize, remove minimum overlapSize. Fixes #481

nicktindall force-pushed the temp/overlap-vs-chunk branch from 80530a3 to f359b12 Compare March 30, 2023 06:46

nicktindall merged commit 9532b87 into develop Mar 30, 2023
1 check passed

nicktindall deleted the temp/overlap-vs-chunk branch March 30, 2023 06:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix offset in acquire, inside() method #498

Fix offset in acquire, inside() method #498

nicktindall commented Mar 21, 2023 •

edited

nicktindall Mar 21, 2023 •

edited

nicktindall Mar 21, 2023 •

edited

nicktindall Mar 21, 2023

peter-lawrey Mar 29, 2023

nicktindall Mar 21, 2023

nicktindall Mar 21, 2023

nicktindall Mar 21, 2023

nicktindall Mar 21, 2023

nicktindall Mar 21, 2023

nicktindall Mar 21, 2023 •

edited

JerryShea Mar 28, 2023

sonarcloud bot commented Mar 30, 2023

Fix offset in acquire, inside() method #498

Fix offset in acquire, inside() method #498

Conversation

nicktindall commented Mar 21, 2023 • edited

nicktindall Mar 21, 2023 • edited

Choose a reason for hiding this comment

nicktindall Mar 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicktindall Mar 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Mar 30, 2023

nicktindall commented Mar 21, 2023 •

edited

nicktindall Mar 21, 2023 •

edited

nicktindall Mar 21, 2023 •

edited

nicktindall Mar 21, 2023 •

edited