Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Merge pull request #805 from jakewins/no-lowlevel-exception-leakage

InvalidRecordException -> NotFoundException
  • Loading branch information...
commit 50b3f9a4daeecc0aa4cfbd39c5999a84b659d001 2 parents ce4d2b8 + e347ac5
Mattias Persson tinwelint authored
2  kernel/src/main/java/org/neo4j/kernel/impl/core/IntArrayIterator.java
View
@@ -145,7 +145,7 @@ else if ( fromNode.getMoreRelationships( nodeManager ) ||
break;
}
}
- } while ( currentTypeIterator.hasNext() );
+ } while ( currentTypeIterator.hasNext() );
// no next element found
return null;
}
41 kernel/src/main/java/org/neo4j/kernel/impl/core/NodeImpl.java
View
@@ -38,6 +38,7 @@
import org.neo4j.kernel.impl.cache.SizeOfs;
import org.neo4j.kernel.impl.core.LockReleaser.CowEntityElement;
import org.neo4j.kernel.impl.core.LockReleaser.PrimitiveElement;
+import org.neo4j.kernel.impl.nioneo.store.InvalidRecordException;
import org.neo4j.kernel.impl.nioneo.store.PropertyData;
import org.neo4j.kernel.impl.nioneo.store.Record;
import org.neo4j.kernel.impl.transaction.LockType;
@@ -90,7 +91,7 @@ public int size()
}
return size;
}
-
+
@Override
public int hashCode()
{
@@ -347,7 +348,16 @@ private void loadInitialRelationships( NodeManager nodeManager )
{
if ( relationships == null )
{
- relChainPosition = nodeManager.getRelationshipChainPosition( this );
+ try
+ {
+ relChainPosition = nodeManager.getRelationshipChainPosition( this );
+ }
+ catch ( InvalidRecordException e )
+ {
+ throw new NotFoundException( asProxy( nodeManager ) +
+ " concurrently deleted while loading its relationships?", e );
+ }
+
ArrayMap<String,RelIdArray> tmpRelMap = new ArrayMap<String,RelIdArray>();
rels = getMoreRelationships( nodeManager, tmpRelMap );
this.relationships = toRelIdArray( tmpRelMap );
@@ -386,7 +396,6 @@ protected void updateSize( NodeManager nodeManager )
return result;
}
-// private Triplet<ArrayMap<String,RelIdArray>,Map<Long,RelationshipImpl>,Long> getMoreRelationships(
// NodeManager nodeManager, ArrayMap<String,RelIdArray> tmpRelMap )
private Triplet<ArrayMap<String,RelIdArray>,List<RelationshipImpl>,Long> getMoreRelationships(
NodeManager nodeManager, ArrayMap<String,RelIdArray> tmpRelMap )
@@ -395,10 +404,10 @@ protected void updateSize( NodeManager nodeManager )
{
return null;
}
-// Triplet<ArrayMap<String,RelIdArray>,Map<Long,RelationshipImpl>,Long> rels =
-// nodeManager.getMoreRelationships( this );
- Triplet<ArrayMap<String,RelIdArray>,List<RelationshipImpl>,Long> rels =
- nodeManager.getMoreRelationships( this );
+ Triplet<ArrayMap<String,RelIdArray>,List<RelationshipImpl>,Long> rels;
+
+ rels = loadMoreRelationshipsFromNodeManager(nodeManager);
+
ArrayMap<String,RelIdArray> addMap = rels.first();
if ( addMap.size() == 0 )
{
@@ -433,7 +442,6 @@ boolean hasMoreRelationshipsToLoad()
boolean getMoreRelationships( NodeManager nodeManager )
{
-// Triplet<ArrayMap<String,RelIdArray>,Map<Long,RelationshipImpl>,Long> rels;
Triplet<ArrayMap<String,RelIdArray>,List<RelationshipImpl>,Long> rels;
if ( !hasMoreRelationshipsToLoad() )
{
@@ -445,7 +453,7 @@ boolean getMoreRelationships( NodeManager nodeManager )
{
return false;
}
- rels = nodeManager.getMoreRelationships( this );
+ rels = loadMoreRelationshipsFromNodeManager(nodeManager);
ArrayMap<String,RelIdArray> addMap = rels.first();
if ( addMap.size() == 0 )
{
@@ -477,6 +485,19 @@ boolean getMoreRelationships( NodeManager nodeManager )
return true;
}
+ private Triplet<ArrayMap<String, RelIdArray>, List<RelationshipImpl>, Long>
+ loadMoreRelationshipsFromNodeManager( NodeManager nodeManager )
+ {
+ try
+ {
+ return nodeManager.getMoreRelationships( this );
+ } catch(InvalidRecordException e)
+ {
+ throw new NotFoundException( "Unable to load one or more relationships from " + asProxy( nodeManager ) +
+ ". This usually happens when relationships are deleted by someone else just as we are about to load them. Please try again.", e );
+ }
+ }
+
private RelIdArray getRelIdArray( String type )
{
// Concurrency-wise it's ok even if the relationships variable
@@ -496,7 +517,7 @@ private void putRelIdArray( RelIdArray addRels )
{
// we don't do size update here, instead performed in lockRelaser
// when calling commitRelationshipMaps and in getMoreRelationships
-
+
// precondition: called under synchronization
// make a local reference to the array to avoid multiple read barrier hits
29 kernel/src/main/java/org/neo4j/kernel/impl/core/Primitive.java
View
@@ -26,6 +26,7 @@
import org.neo4j.graphdb.PropertyContainer;
import org.neo4j.kernel.impl.core.LockReleaser.CowEntityElement;
import org.neo4j.kernel.impl.core.LockReleaser.PrimitiveElement;
+import org.neo4j.kernel.impl.nioneo.store.InvalidRecordException;
import org.neo4j.kernel.impl.nioneo.store.PropertyData;
import org.neo4j.kernel.impl.transaction.LockType;
import org.neo4j.kernel.impl.util.ArrayMap;
@@ -64,7 +65,7 @@ protected abstract void removeProperty( NodeManager nodeManager,
protected abstract void commitPropertyMaps(
ArrayMap<Integer,PropertyData> cowPropertyAddMap,
ArrayMap<Integer,PropertyData> cowPropertyRemoveMap, long firstProp, NodeManager nodeManager );
-
+
@Override
public int hashCode()
{
@@ -563,19 +564,33 @@ private Object getPropertyValue( NodeManager nodeManager, PropertyData property
private void ensureFullProperties( NodeManager nodeManager )
{
- // double checked locking
- if ( allProperties() == null ) synchronized ( this )
- {
- if ( allProperties() == null ) setProperties( loadProperties( nodeManager, false ), nodeManager );
- }
+ ensureFullProperties( nodeManager, /* light = */ false );
}
private void ensureFullLightProperties( NodeManager nodeManager )
{
+ ensureFullProperties( nodeManager, /* light = */ true );
+ }
+
+ private void ensureFullProperties(NodeManager nodeManager, boolean light )
+ {
// double checked locking
if ( allProperties() == null ) synchronized ( this )
{
- if ( allProperties() == null ) setProperties( loadProperties( nodeManager, true ), nodeManager );
+ if ( allProperties() == null )
+ {
+ try
+ {
+ ArrayMap<Integer, PropertyData> loadedProperties = loadProperties( nodeManager, light );
+ setProperties( loadedProperties, nodeManager );
+ }
+ catch ( InvalidRecordException e )
+ {
+ throw new NotFoundException( asProxy( nodeManager ) + " not found. This can be because someone " +
+ "else deleted this entity while we were trying to read properties from it, or because of " +
+ "concurrent modification of other properties on this entity. The problem should be temporary.", e );
+ }
+ }
}
}
25 kernel/src/main/java/org/neo4j/kernel/impl/util/DebugUtil.java
View
@@ -92,10 +92,11 @@ public static boolean currentStackTraceContains( Class<?> cls, String method )
public static class StackTracer
{
private final Map<Stack, AtomicInteger> uniqueStackTraces = new HashMap<Stack, AtomicInteger>();
+ private boolean considerMessages = true;
public void add( Throwable t )
{
- Stack key = new Stack( t );
+ Stack key = new Stack( t, considerMessages );
AtomicInteger count = uniqueStackTraces.get( key );
if ( count == null )
{
@@ -130,23 +131,32 @@ public void run()
} );
return this;
}
+
+ public StackTracer ignoreMessages()
+ {
+ considerMessages = false;
+ return this;
+ }
}
private static class Stack
{
private final Throwable stackTrace;
private final StackTraceElement[] elements;
+ private boolean considerMessage;
- Stack( Throwable stackTrace )
+ Stack( Throwable stackTrace, boolean considerMessage )
{
this.stackTrace = stackTrace;
+ this.considerMessage = considerMessage;
this.elements = stackTrace.getStackTrace();
}
@Override
public int hashCode()
{
- int hashCode = stackTrace.getMessage().hashCode();
+ int hashCode = stackTrace.getMessage() == null || !considerMessage ? 31 :
+ stackTrace.getMessage().hashCode();
for ( StackTraceElement element : stackTrace.getStackTrace() )
hashCode = hashCode * 9 + element.hashCode();
return hashCode;
@@ -158,7 +168,14 @@ public boolean equals( Object obj )
if ( !( obj instanceof Stack) ) return false;
Stack o = (Stack) obj;
- if ( !stackTrace.getMessage().equals( o.stackTrace.getMessage() ) ) return false;
+ if ( considerMessage )
+ {
+ if ( stackTrace.getMessage() == null )
+ {
+ if ( o.stackTrace.getMessage() != null ) return false;
+ }
+ else if ( !stackTrace.getMessage().equals( o.stackTrace.getMessage() ) ) return false;
+ }
if ( elements.length != o.elements.length ) return false;
for ( int i = 0; i < elements.length; i++ )
if ( !elements[i].equals( o.elements[i] ) ) return false;
189 kernel/src/test/java/org/neo4j/kernel/impl/core/TestOperationsOnDeletedPrimitive.java
View
@@ -0,0 +1,189 @@
+/**
+ * Copyright (c) 2002-2012 "Neo Technology,"
+ * Network Engine for Objects in Lund AB [http://neotechnology.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Neo4j is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+package org.neo4j.kernel.impl.core;
+
+import static org.mockito.Mockito.*;
+
+import org.junit.Test;
+import org.neo4j.graphdb.NotFoundException;
+import org.neo4j.graphdb.PropertyContainer;
+import org.neo4j.kernel.impl.nioneo.store.InvalidRecordException;
+import org.neo4j.kernel.impl.nioneo.store.PropertyData;
+import org.neo4j.kernel.impl.util.ArrayMap;
+
+/**
+ * To cover cases where either the primitive that a property belongs to has
+ * been removed, as well as cases where we are reading a property chain and
+ * someone suddenly removes items in the chain.
+ *
+ * The second case, with inconsistent chains, should not be handled like this later on,
+ * we don't want to throw NotFoundException to the user. Rather, we want to introduce appropriate
+ * read locks such that we never read a property chain in an inconsistent state.
+ *
+ * The same thing applies to reading relationship chains.
+ *
+ * Once we modify HA to take appropriate locks while applying transactions to slaves,
+ * this should be ready to be implemented.
+ */
+public class TestOperationsOnDeletedPrimitive
+{
+ private NodeManager nodeManager = mock(NodeManager.class);
+ private PropertyContainer propertyContainer = mock( PropertyContainer.class );
+ Primitive primitive = new PrimitiveThatHasActuallyBeenDeleted( false );
+
+ @Test(expected = NotFoundException.class)
+ public void shouldThrowNotFoundOnGetPropertyWithDefaultOnDeletedEntity() throws Exception
+ {
+ primitive.getProperty( nodeManager, "the_property", new Object() );
+ }
+
+ @Test(expected = NotFoundException.class)
+ public void shouldThrowNotFoundExceptionOnGetPropertyOnDeletedEntity() throws Exception
+ {
+ primitive.getProperty( nodeManager, "the_property" );
+ }
+
+ @Test(expected = NotFoundException.class)
+ public void shouldThrowNotFoundExceptionOnSetPropertyOnDeletedEntity() throws Exception
+ {
+ primitive.setProperty( nodeManager, propertyContainer, "the_property", "the value" );
+ }
+
+ @Test(expected = NotFoundException.class)
+ public void shouldThrowNotFoundExceptionOnRemovePropertyOnDeletedEntity() throws Exception
+ {
+ primitive.removeProperty( nodeManager, propertyContainer, "the_property" );
+ }
+
+ @Test(expected = NotFoundException.class)
+ public void shouldThrowNotFoundExceptionOnGetPropertyKeysOnDeletedEntity() throws Exception
+ {
+ primitive.getPropertyKeys( nodeManager );
+ }
+
+ @Test(expected = NotFoundException.class)
+ public void shouldThrowNotFoundExceptionOnGetPropertyValuesOnDeletedEntity() throws Exception
+ {
+ primitive.getPropertyValues( nodeManager );
+ }
+
+ @Test(expected = NotFoundException.class)
+ public void shouldThrowNotFoundExceptionOnHasPropertyOnDeletedEntity() throws Exception
+ {
+ primitive.hasProperty( nodeManager, "the_property" );
+ }
+
+ @Test(expected = NotFoundException.class)
+ public void shouldThrowNotFoundExceptionOnGetAllCommittedPropertiesOnDeletedEntity() throws Exception
+ {
+ primitive.getAllCommittedProperties( nodeManager );
+ }
+
+ @Test(expected = NotFoundException.class)
+ public void shouldThrowNotFoundExceptionOnGetCommittedPropertyValueOnDeletedEntity() throws Exception
+ {
+ primitive.getCommittedPropertyValue( nodeManager, "the_key" );
+ }
+
+ // Test utils
+
+
+ private class PrimitiveThatHasActuallyBeenDeleted extends Primitive
+ {
+
+ PrimitiveThatHasActuallyBeenDeleted( boolean newPrimitive )
+ {
+ super( newPrimitive );
+ }
+
+ // Because we have been deleted, this always throws invalidRecordException
+ @Override
+ protected ArrayMap<Integer, PropertyData> loadProperties( NodeManager nodeManager, boolean light )
+ {
+ throw new InvalidRecordException( "I have been deleted, remember!" );
+ }
+
+ @Override
+ protected PropertyData changeProperty( NodeManager nodeManager, PropertyData property, Object value )
+ {
+ return null;
+ }
+
+ @Override
+ protected PropertyData addProperty( NodeManager nodeManager, PropertyIndex index, Object value )
+ {
+ return null;
+ }
+
+ @Override
+ protected void removeProperty( NodeManager nodeManager, PropertyData property )
+ {
+ }
+
+ @Override
+ public long getId()
+ {
+ return 0;
+ }
+
+ @Override
+ protected void setEmptyProperties()
+ {
+ }
+
+ @Override
+ protected PropertyData[] allProperties()
+ {
+ return null;
+ }
+
+ @Override
+ protected PropertyData getPropertyForIndex( int keyId )
+ {
+ return null;
+ }
+
+ @Override
+ protected void setProperties( ArrayMap<Integer, PropertyData> properties, NodeManager nodeManager )
+ {
+ }
+
+ @Override
+ protected void commitPropertyMaps( ArrayMap<Integer, PropertyData> cowPropertyAddMap, ArrayMap<Integer,
+ PropertyData> cowPropertyRemoveMap, long firstProp, NodeManager nodeManager )
+ {
+ }
+
+ @Override
+ public LockReleaser.CowEntityElement getEntityElement( LockReleaser.PrimitiveElement element, boolean create )
+ {
+ return null;
+ }
+
+ @Override
+ PropertyContainer asProxy( NodeManager nm )
+ {
+ PropertyContainer mockContainer = mock(PropertyContainer.class);
+ when( mockContainer.toString() ).thenReturn( "MockedEntity[1337]" );
+ return mockContainer;
+ }
+ }
+
+}
93 kernel/src/test/java/org/neo4j/kernel/impl/core/TestOperationsOnDeletedRelationships.java
View
@@ -0,0 +1,93 @@
+/**
+ * Copyright (c) 2002-2012 "Neo Technology,"
+ * Network Engine for Objects in Lund AB [http://neotechnology.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Neo4j is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+package org.neo4j.kernel.impl.core;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.nullValue;
+import static org.hamcrest.core.IsNot.not;
+import static org.junit.Assert.*;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.*;
+
+import org.junit.Test;
+import org.neo4j.graphdb.NotFoundException;
+import org.neo4j.kernel.impl.nioneo.store.InvalidRecordException;
+import org.neo4j.kernel.impl.util.RelIdArray;
+
+public class TestOperationsOnDeletedRelationships
+{
+
+ // Should it really do this? Wouldn't it be better if we could recover from a a relationship suddenly
+ // missing in the chain? Perhaps that is really hard to do though.
+ @Test
+ public void shouldThrowNotFoundOnGetAllRelationshipsWhenRelationshipConcurrentlyDeleted() throws Exception
+ {
+ // Given
+ NodeImpl nodeImpl = new NodeImpl( 1337l, 0l, 0l, false );
+ NodeManager nodeManager = mock(NodeManager.class);
+ Throwable exceptionCaught = null;
+
+ // Given something tries to load relationships, throw InvalidRecordException
+ when( nodeManager.getMoreRelationships( any( NodeImpl.class ) ) ).thenThrow( new InvalidRecordException( "LURING!" ) );
+
+ // When
+ try {
+ nodeImpl.getAllRelationships( nodeManager, RelIdArray.DirectionWrapper.BOTH );
+ } catch(Throwable e)
+ {
+ exceptionCaught = e;
+ }
+
+ // Then
+ assertThat(exceptionCaught, not(nullValue()));
+ assertThat( exceptionCaught, is( NotFoundException.class) );
+ }
+
+ @Test
+ public void shouldThrowNotFoundWhenIteratingOverDeletedRelationship() throws Exception
+ {
+ // Given
+ NodeImpl fromNode = new NodeImpl( 1337l, 0l, 0l, false );
+ NodeManager nodeManager = mock(NodeManager.class);
+ Throwable exceptionCaught = null;
+
+ // This makes fromNode think there are more relationships to be loaded
+ fromNode.setRelChainPosition( 1337l );
+
+ // This makes nodeManager pretend that relationships have been deleted
+ when( nodeManager.getMoreRelationships( any( NodeImpl.class ) ) ).thenThrow( new InvalidRecordException(
+ "LURING!" ) );
+
+
+ // When
+ try
+ {
+ fromNode.getMoreRelationships( nodeManager );
+ } catch(Throwable e)
+ {
+ exceptionCaught = e;
+ }
+
+ // Then
+ assertThat( exceptionCaught, not( nullValue() ) );
+ assertThat( exceptionCaught, is( NotFoundException.class) );
+ }
+
+}
Please sign in to comment.
Something went wrong with that request. Please try again.