Skip to content

Commit

Permalink
Fix shared ValuesContainer corruption on ValuesMap.clear() + moar tests
Browse files Browse the repository at this point in the history 
The problem was that single ValuesContainer instance had been shared
between all ValuesMap instances within tx scope, and at the same time
ValuesMap.clear() was actually invoking ValuesContainer.clear() on that
shared instance.
  • Loading branch information
@andreikoval
andreikoval committed Sep 20, 2018
1 parent c2006c1 commit 4709b50
Show file tree
Hide file tree
Showing 5 changed files with 85 additions and 45 deletions.
10 changes: 0 additions & 10 deletions ...unity/kernel/src/main/java/org/neo4j/kernel/impl/api/state/AppendOnlyValuesContainer.java
View file
Expand Up @@ -190,16 +190,6 @@ public Value remove( long ref )
return removed;
}

@Override
public void clear()
{
assertNotClosed();
allocated.forEach( Memory::free );
allocated.clear();
chunks.clear();
currentChunk = addNewChunk( chunkSize );
}

@Override
public void close()
{
Expand Down
6 changes: 0 additions & 6 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/api/state/ValuesContainer.java
View file
Expand Up @@ -55,10 +55,4 @@ public interface ValuesContainer extends Resource
*/
@Nonnull
Value remove( long ref );

/**
* Clears the container and releases all underlying resources.
* @throws IllegalStateException if container is closed
*/
void clear();
}
1 change: 0 additions & 1 deletion community/kernel/src/main/java/org/neo4j/kernel/impl/api/state/ValuesMap.java
View file
Expand Up @@ -373,7 +373,6 @@ public Value remove( long key )
@Override
public void clear()
{
valuesContainer.clear();
refs.clear();
}

Expand Down
17 changes: 0 additions & 17 deletions ...y/kernel/src/test/java/org/neo4j/kernel/impl/api/state/AppendOnlyValuesContainerTest.java
View file
Expand Up @@ -227,22 +227,6 @@ void remove()
assertThrows( IllegalArgumentException.class, () -> container.get( ref ) );
}

@Test
void clear()
{
final Value value1 = stringValue( "foo" );
final Value value2 = stringValue( "bar" );
final long ref1 = container.add( value1 );
final long ref2 = container.add( value2 );
assertEquals( value1, container.get( ref1 ) );
assertEquals( value2, container.get( ref2 ) );

container.clear();

assertThrows( IllegalArgumentException.class, () -> container.get( ref1 ) );
assertThrows( IllegalArgumentException.class, () -> container.get( ref2 ) );
}

@Test
void valueSizeExceedsChunkSize()
{
Expand All @@ -265,7 +249,6 @@ void close()
assertThrows( IllegalStateException.class, () -> container2.add( intValue( 1 ) ) );
assertThrows( IllegalStateException.class, () -> container2.get( ref ) );
assertThrows( IllegalStateException.class, () -> container2.remove( ref ) );
assertThrows( IllegalStateException.class, container2::clear );
assertThrows( IllegalStateException.class, container2::close );
}

Expand Down
96 changes: 85 additions & 11 deletions community/kernel/src/test/java/org/neo4j/kernel/impl/api/state/ValuesMapTest.java
View file
Expand Up @@ -26,10 +26,18 @@
import org.eclipse.collections.api.block.procedure.Procedure;
import org.eclipse.collections.api.block.procedure.primitive.LongObjectProcedure;
import org.eclipse.collections.api.block.procedure.primitive.LongProcedure;
import org.eclipse.collections.api.map.primitive.MutableLongObjectMap;
import org.eclipse.collections.impl.map.mutable.primitive.LongLongHashMap;
import org.eclipse.collections.impl.map.mutable.primitive.LongObjectHashMap;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;

import java.util.ArrayList;
import java.util.List;

import org.neo4j.test.extension.Inject;
import org.neo4j.test.extension.RandomExtension;
import org.neo4j.test.rule.RandomRule;
import org.neo4j.values.storable.Value;

import static org.junit.jupiter.api.Assertions.assertEquals;
Expand All @@ -45,9 +53,13 @@
import static org.mockito.internal.verification.VerificationModeFactory.times;
import static org.neo4j.values.storable.Values.intValue;

@ExtendWith( RandomExtension.class )
class ValuesMapTest
{
private ValuesMap map = newMap();
@Inject
private RandomRule rnd;

private final ValuesMap map = newMap();

@Test
void putGet()
Expand Down Expand Up @@ -174,15 +186,15 @@ void putOverwrite()
{
map.putAll( LongObjectHashMap.newWithKeysValues( 0, intValue( 10 ), 1, intValue( 11 ), 2, intValue( 12 ) ) );

assertEquals( intValue(10), map.get( 0 ) );
assertEquals( intValue(11), map.get( 1 ) );
assertEquals( intValue(12), map.get( 2 ) );
assertEquals( intValue( 10 ), map.get( 0 ) );
assertEquals( intValue( 11 ), map.get( 1 ) );
assertEquals( intValue( 12 ), map.get( 2 ) );

map.putAll( LongObjectHashMap.newWithKeysValues( 0, intValue( 20 ), 1, intValue( 21 ), 2, intValue( 22 ) ) );

assertEquals( intValue(20), map.get( 0 ) );
assertEquals( intValue(21), map.get( 1 ) );
assertEquals( intValue(22), map.get( 2 ) );
assertEquals( intValue( 20 ), map.get( 0 ) );
assertEquals( intValue( 21 ), map.get( 1 ) );
assertEquals( intValue( 22 ), map.get( 2 ) );
}

@Test
Expand Down Expand Up @@ -214,13 +226,13 @@ void containsKey()
assertFalse( map.containsKey( 1 ) );
assertFalse( map.containsKey( 2 ) );

map.put( 0, intValue(10) );
map.put( 0, intValue( 10 ) );
assertTrue( map.containsKey( 0 ) );

map.put( 1, intValue(11) );
map.put( 1, intValue( 11 ) );
assertTrue( map.containsKey( 1 ) );

map.put( 2, intValue(12) );
map.put( 2, intValue( 12 ) );
assertTrue( map.containsKey( 2 ) );
}

Expand Down Expand Up @@ -283,8 +295,70 @@ void clear()
assertEquals( 0, map.size() );
}

@Test
void randomizedWithSharedValuesContainer()
{
final int MAPS = 13;
final int COUNT = 10000 + rnd.nextInt( 1000 );

final AppendOnlyValuesContainer valuesContainer = new AppendOnlyValuesContainer( new TestMemoryAllocator() );

final List<ValuesMap> actualMaps = new ArrayList<>();
final List<MutableLongObjectMap<Value>> expectedMaps = new ArrayList<>();

for ( int i = 0; i < MAPS; i++ )
{
actualMaps.add( newMap( valuesContainer ) );
expectedMaps.add( new LongObjectHashMap<>() );
}

for ( int i = 0; i < MAPS; i++ )
{
put( COUNT, actualMaps.get( i ), expectedMaps.get( i ) );
}

for ( int i = 0; i < MAPS; i++ )
{
remove( COUNT, actualMaps.get( i ), expectedMaps.get( i ) );
}

for ( int i = 0; i < MAPS; i++ )
{
final MutableLongObjectMap<Value> expected = expectedMaps.get( i );
final ValuesMap actual = actualMaps.get( i );
expected.forEachKeyValue( ( k, v ) -> assertEquals( v, actual.get( k ) ) );
}
}

private void remove( int count, ValuesMap actualMap, MutableLongObjectMap<Value> expectedMap )
{
for ( int i = 0; i < count / 2; i++ )
{
final long key = rnd.nextLong( count );
final Value value = rnd.randomValues().nextValue();
actualMap.put( key, value );
expectedMap.put( key, value );
}
}

private void put( int count, ValuesMap actualMap, MutableLongObjectMap<Value> expectedMap )
{
for ( int i = 0; i < count * 2; i++ )
{
final long key = rnd.nextLong( count );
final Value value = rnd.randomValues().nextValue();
actualMap.put( key, value );
expectedMap.put( key, value );
}
}

private static ValuesMap newMap()
{
return new ValuesMap( new LongLongHashMap(), new AppendOnlyValuesContainer( new TestMemoryAllocator() ) );
return newMap( new AppendOnlyValuesContainer( new TestMemoryAllocator() ) );
}

private static ValuesMap newMap( AppendOnlyValuesContainer valuesContainer )
{
return new ValuesMap( new LongLongHashMap(), valuesContainer );
}
}

0 comments on commit 4709b50

Please sign in to comment.