afs commented on code in PR #4051: URL: https://github.com/apache/jena/pull/4051#discussion_r3610748629
########## jena-db/jena-dboe-base/src/test/java/org/apache/jena/dboe/base/file/TestBlockAccessMappedSegmentState.java: ########## @@ -0,0 +1,196 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.apache.jena.dboe.base.file; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +import java.lang.reflect.Field; +import java.nio.MappedByteBuffer; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import org.apache.jena.atlas.lib.FileOps; +import org.apache.jena.dboe.ConfigTestDBOE; +import org.apache.jena.dboe.base.block.Block; + +/** + * The shared per-segment {@link MappedByteBuffer} in {@link BlockAccessMapped} + * must never have its position/limit mutated by block access. + * + * <p>The previous implementation sliced blocks with a + * position/limit/slice/reset-limit sequence on the shared segment buffer. If + * anything threw between the limit-shrink and the reset (e.g. an + * {@link OutOfMemoryError} during {@code slice()}), the segment buffer was left + * with a shrunken limit and every later access to a higher block in that + * segment failed with {@code IllegalArgumentException: newPosition > limit} + * from {@code Buffer.position(int)} - observed in production as persistent 500s + * on reads of a B+Tree index until restart. The fix slices with the absolute + * {@code slice(index, length)}, which never touches the shared buffer's state. + * These tests lock in that invariant.</p> + */ +public class TestBlockAccessMappedSegmentState { + + private static final int BlockSize = 64; + private static int counter = 0; + + private String filename; + private BlockAccessMapped file; + + @BeforeEach public void before() { + filename = ConfigTestDBOE.getTestingDir() + "/test-segment-state-" + (counter++); + FileOps.deleteSilent(filename); + file = new BlockAccessMapped(filename, BlockSize); + } + + @AfterEach public void after() { + file.close(); + FileOps.deleteSilent(filename); + } + + private Block writePatternBlock(int marker) { + Block b = file.allocate(BlockSize); + for ( int i = 0; i < BlockSize; i++ ) + b.getByteBuffer().put(i, (byte)((marker + i) & 0xFF)); + file.write(b); + return b; + } + + private MappedByteBuffer segmentBuffer(int seg) throws Exception { + Field f = BlockAccessMapped.class.getDeclaredField("segments"); + f.setAccessible(true); + MappedByteBuffer[] segments = (MappedByteBuffer[])f.get(file); + return segments[seg]; + } + + @Test + public void sharedSegmentBufferStateUntouchedByAccess() throws Exception { + final int numBlocks = 20; + Block[] blocks = new Block[numBlocks]; + for ( int i = 0; i < numBlocks; i++ ) + blocks[i] = writePatternBlock(i * 7); + + // Interleave reads in an order that, with the old position/limit + // slicing, walked the shared buffer's position and limit up and down. + // End on a high block: the old implementation left position at that + // block's segment offset. + for ( int i = 0; i < numBlocks; i++ ) + file.read(blocks[i].getId().longValue()); + file.read(blocks[0].getId().longValue()); + file.read(blocks[numBlocks - 1].getId().longValue()); + + MappedByteBuffer seg0 = segmentBuffer(0); + assertNotNull(seg0, "segment 0 should be mapped after block access"); + System.out.printf("segment 0 after %d writes + %d reads: position=%d limit=%d capacity=%d%n", + numBlocks, numBlocks + 2, seg0.position(), seg0.limit(), seg0.capacity()); + + // A shrunken limit is the "newPosition > limit" poisoning vector. + assertEquals(seg0.capacity(), seg0.limit(), + "shared segment buffer limit was shrunk by block access - " + + "an exception mid-access would poison all higher blocks in the segment"); + assertEquals(0, seg0.position(), + "shared segment buffer position was mutated by block access"); + } + + @Test + public void blockRoundTripAfterMixedAccess() { + final int numBlocks = 8; + Block[] written = new Block[numBlocks]; + for ( int i = 0; i < numBlocks; i++ ) + written[i] = writePatternBlock(i * 31); + + for ( int i = 0; i < numBlocks; i++ ) { + Block back = file.read(written[i].getId().longValue()); + assertEquals(0, back.getByteBuffer().position(), "returned block slice should start at position 0"); + assertEquals(BlockSize, back.getByteBuffer().capacity(), "returned block slice capacity"); + for ( int j = 0; j < BlockSize; j++ ) { + byte expected = (byte)((i * 31 + j) & 0xFF); + byte actual = back.getByteBuffer().get(j); + if ( expected != actual ) + System.out.printf("MISMATCH block=%d byte=%d expected=%02x actual=%02x%n", + i, j, expected, actual); + assertEquals(expected, actual, + String.format("content mismatch: block=%d byte=%d", i, j)); + } + } + } + + /** + * Deterministic reproduction of the production failure. Review Comment: Better to refer to the GH issue. No need to take the description out of context. This code isn't Fuseki specific. ########## jena-db/jena-dboe-base/src/test/java/org/apache/jena/dboe/base/file/TestBlockAccessMappedSegmentState.java: ########## @@ -0,0 +1,196 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.apache.jena.dboe.base.file; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +import java.lang.reflect.Field; +import java.nio.MappedByteBuffer; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import org.apache.jena.atlas.lib.FileOps; +import org.apache.jena.dboe.ConfigTestDBOE; +import org.apache.jena.dboe.base.block.Block; + +/** + * The shared per-segment {@link MappedByteBuffer} in {@link BlockAccessMapped} + * must never have its position/limit mutated by block access. + * + * <p>The previous implementation sliced blocks with a + * position/limit/slice/reset-limit sequence on the shared segment buffer. If + * anything threw between the limit-shrink and the reset (e.g. an + * {@link OutOfMemoryError} during {@code slice()}), the segment buffer was left + * with a shrunken limit and every later access to a higher block in that + * segment failed with {@code IllegalArgumentException: newPosition > limit} + * from {@code Buffer.position(int)} - observed in production as persistent 500s + * on reads of a B+Tree index until restart. The fix slices with the absolute + * {@code slice(index, length)}, which never touches the shared buffer's state. + * These tests lock in that invariant.</p> + */ +public class TestBlockAccessMappedSegmentState { + + private static final int BlockSize = 64; + private static int counter = 0; + + private String filename; + private BlockAccessMapped file; + + @BeforeEach public void before() { + filename = ConfigTestDBOE.getTestingDir() + "/test-segment-state-" + (counter++); + FileOps.deleteSilent(filename); + file = new BlockAccessMapped(filename, BlockSize); + } + + @AfterEach public void after() { + file.close(); + FileOps.deleteSilent(filename); + } + + private Block writePatternBlock(int marker) { + Block b = file.allocate(BlockSize); + for ( int i = 0; i < BlockSize; i++ ) + b.getByteBuffer().put(i, (byte)((marker + i) & 0xFF)); + file.write(b); + return b; + } + + private MappedByteBuffer segmentBuffer(int seg) throws Exception { + Field f = BlockAccessMapped.class.getDeclaredField("segments"); + f.setAccessible(true); + MappedByteBuffer[] segments = (MappedByteBuffer[])f.get(file); + return segments[seg]; + } + + @Test + public void sharedSegmentBufferStateUntouchedByAccess() throws Exception { + final int numBlocks = 20; + Block[] blocks = new Block[numBlocks]; + for ( int i = 0; i < numBlocks; i++ ) + blocks[i] = writePatternBlock(i * 7); + + // Interleave reads in an order that, with the old position/limit + // slicing, walked the shared buffer's position and limit up and down. + // End on a high block: the old implementation left position at that + // block's segment offset. + for ( int i = 0; i < numBlocks; i++ ) + file.read(blocks[i].getId().longValue()); + file.read(blocks[0].getId().longValue()); + file.read(blocks[numBlocks - 1].getId().longValue()); + + MappedByteBuffer seg0 = segmentBuffer(0); + assertNotNull(seg0, "segment 0 should be mapped after block access"); + System.out.printf("segment 0 after %d writes + %d reads: position=%d limit=%d capacity=%d%n", Review Comment: Tests shouldn't write to `System.out`. These will need to be removed when finalizing the PR. ########## jena-db/jena-dboe-base/src/main/java/org/apache/jena/dboe/base/file/BlockAccessMapped.java: ########## @@ -149,20 +149,19 @@ private ByteBuffer getByteBuffer(long _id) { synchronized (this) { try { - // Need to put the alloc AND the slice/reset inside a sync. + // Need to put the alloc AND the slice inside a sync. ByteBuffer segBuffer = allocSegment(seg); - // Now slice the buffer to get the ByteBuffer to return - segBuffer.position(segOff); - segBuffer.limit(segOff+blockSize); - ByteBuffer dst = segBuffer.slice(); - - // And then reset limit to max for segment. - segBuffer.limit(segBuffer.capacity()); + // Slice from a cleared duplicate: never touches the shared + // segment buffer's position/limit, and does not depend on them + // either. The old position/limit/slice/reset sequence could + // leave a shrunken limit on the shared buffer if anything threw + // before the reset, poisoning every later access to a higher + // block in the segment ("newPosition > limit"). + ByteBuffer dst = segBuffer.duplicate().clear().slice(segOff, blockSize); Review Comment: Both `duplicate` and `slice(segOff, blockSize)` create a new independent ByteBuffer objects with a call to DirectByteBuffer. So that's two objects. The root cause is mutating `segBuffer` to pass information to `slice()` (method with 0 arguments), i.e. set to size 64, reset to 8388608. For a short while, `segBuffer` limit is 64. This can be done with a single object creation `segBuffer.slice(segOff, blockSize)`. This does not change `segBuffer` at any point. Test `poisonedSegmentLimit_readsStillWork` fails in the check reading limit() of block(0) but the test the simulates the effect observed, not how it happened. poisonedSegmentLimit_readsStillWork: ``` MappedByteBuffer seg0 = segmentBuffer(0); seg0.limit(BlockSize); ``` `seg0` is the returned fresh MappedByteBuffer `dst` and block 0 is always size 64. It is not misread as 8388608 because with `segBuffer.slice(segOff, blockSize)`, the limit is always the correct 64. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
