Remove synchronized from turtle inventory code

These should never be called off the server thread, so this doesn't make
much difference.

src/main/java/dan200/computercraft/api/turtle/ITurtleAccess.java

@@ -145,7 +145,9 @@ public interface ITurtleAccess
     GameProfile getOwningPlayer();
 
     /**
-     * Get the inventory of this turtle
+     * Get the inventory of this turtle.
+     *
+     * Note: this inventory should only be accessed and modified on the server thread.
      *
      * @return This turtle's inventory
      * @see #getItemHandler()
@@ -156,6 +158,8 @@ public interface ITurtleAccess
     /**
      * Get the inventory of this turtle as an {@link IItemHandlerModifiable}.
      *
+     * Note: this inventory should only be accessed and modified on the server thread.
+     *
      * @return This turtle's inventory
      * @see #getInventory()
      * @see IItemHandlerModifiable

src/main/java/dan200/computercraft/shared/turtle/FurnaceRefuelHandler.java

@@ -33,7 +33,7 @@ public int refuel( @Nonnull ITurtleAccess turtle, @Nonnull ItemStack currentStac
         int fuelSpaceLeft = turtle.getFuelLimit() - turtle.getFuelLevel();
         int fuelPerItem = getFuelPerItem( turtle.getItemHandler().getStackInSlot( slot ) );
         int fuelItemLimit = (int) Math.ceil( fuelSpaceLeft / (double) fuelPerItem );
-        if ( limit > fuelItemLimit ) limit = fuelItemLimit;
+        if( limit > fuelItemLimit ) limit = fuelItemLimit;
 
         ItemStack stack = turtle.getItemHandler().extractItem( slot, limit, false );
         int fuelToGive = fuelPerItem * stack.getCount();

src/main/java/dan200/computercraft/shared/turtle/apis/TurtleAPI.java

@@ -334,9 +334,11 @@ public Object[] callMethod( @Nonnull ILuaContext context, int method, @Nonnull O
                 return tryCommand( context, new TurtleInspectCommand( InteractDirection.Up ) );
             case 40: // inspectDown
                 return tryCommand( context, new TurtleInspectCommand( InteractDirection.Down ) );
-            case 41:
+            case 41: // getItemDetail
             {
-                // getItemDetail
+                // FIXME: There's a race condition here if the stack is being modified (mutating NBT, etc...)
+                //  on another thread. The obvious solution is to move this into a command, but some programs rely
+                //  on this having a 0-tick delay.
                 int slot = parseOptionalSlotNumber( args, 0, m_turtle.getSelectedSlot() );
                 ItemStack stack = m_turtle.getInventory().getStackInSlot( slot );
                 if( stack.isEmpty() ) return new Object[] { null };

src/main/java/dan200/computercraft/shared/turtle/blocks/TileTurtle.java

@@ -43,19 +43,16 @@
 
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
+import java.util.Collections;
 
 import static net.minecraftforge.items.CapabilityItemHandler.ITEM_HANDLER_CAPABILITY;
 
 public class TileTurtle extends TileComputerBase implements ITurtleTile, DefaultInventory
 {
-    // Statics
-
     public static final int INVENTORY_SIZE = 16;
     public static final int INVENTORY_WIDTH = 4;
     public static final int INVENTORY_HEIGHT = 4;
 
-    // Members
-
     enum MoveState
     {
         NOT_MOVED,
@@ -63,12 +60,12 @@ enum MoveState
         MOVED
     }
 
-    private NonNullList<ItemStack> m_inventory;
+    private final NonNullList<ItemStack> m_inventory = NonNullList.withSize( INVENTORY_SIZE, ItemStack.EMPTY );
-    private NonNullList<ItemStack> m_previousInventory;
+    private final NonNullList<ItemStack> m_previousInventory = NonNullList.withSize( INVENTORY_SIZE, ItemStack.EMPTY );
     private final IItemHandlerModifiable m_itemHandler = new InvWrapper( this );
-    private boolean m_inventoryChanged;
+    private boolean m_inventoryChanged = false;
-    private TurtleBrain m_brain;
+    private TurtleBrain m_brain = new TurtleBrain( this );
-    private MoveState m_moveState;
+    private MoveState m_moveState = MoveState.NOT_MOVED;
     private ComputerFamily m_family;
 
     public TileTurtle()
@@ -78,15 +75,10 @@ public TileTurtle()
 
     public TileTurtle( ComputerFamily family )
     {
-        m_inventory = NonNullList.withSize( INVENTORY_SIZE, ItemStack.EMPTY );
-        m_previousInventory = NonNullList.withSize( INVENTORY_SIZE, ItemStack.EMPTY );
-        m_inventoryChanged = false;
-        m_brain = new TurtleBrain( this );
-        m_moveState = MoveState.NOT_MOVED;
         m_family = family;
     }
 
-    public boolean hasMoved()
+    private boolean hasMoved()
     {
         return m_moveState == MoveState.MOVED;
     }
@@ -224,18 +216,15 @@ public void update()
     {
         super.update();
         m_brain.update();
-        synchronized( m_inventory )
+        if( !getWorld().isRemote && m_inventoryChanged )
         {
-            if( !getWorld().isRemote && m_inventoryChanged )
+            ServerComputer computer = getServerComputer();
-            {
+            if( computer != null ) computer.queueEvent( "turtle_inventory" );
-                ServerComputer computer = getServerComputer();
-                if( computer != null ) computer.queueEvent( "turtle_inventory" );
 
-                m_inventoryChanged = false;
+            m_inventoryChanged = false;
-                for( int n = 0; n < getSizeInventory(); n++ )
+            for( int n = 0; n < getSizeInventory(); n++ )
-                {
+            {
-                    m_previousInventory.set( n, InventoryUtil.copyItem( getStackInSlot( n ) ) );
+                m_previousInventory.set( n, InventoryUtil.copyItem( getStackInSlot( n ) ) );
-                }
             }
         }
     }
@@ -270,8 +259,8 @@ public void readFromNBT( NBTTagCompound nbt )
 
         // Read inventory
         NBTTagList nbttaglist = nbt.getTagList( "Items", Constants.NBT.TAG_COMPOUND );
-        m_inventory = NonNullList.withSize( INVENTORY_SIZE, ItemStack.EMPTY );
+        m_inventory.clear();
-        m_previousInventory = NonNullList.withSize( INVENTORY_SIZE, ItemStack.EMPTY );
+        m_previousInventory.clear();
         for( int i = 0; i < nbttaglist.tagCount(); i++ )
         {
             NBTTagCompound tag = nbttaglist.getCompoundTagAt( i );
@@ -382,7 +371,7 @@ public ComputerFamily getFamily()
         return m_family;
     }
 
-    public void setOwningPlayer( GameProfile player )
+    void setOwningPlayer( GameProfile player )
     {
         m_brain.setOwningPlayer( player );
         markDirty();
@@ -410,109 +399,76 @@ public boolean isEmpty()
     @Override
     public ItemStack getStackInSlot( int slot )
     {
-        if( slot >= 0 && slot < INVENTORY_SIZE )
+        return slot >= 0 && slot < INVENTORY_SIZE ? m_inventory.get( slot ) : ItemStack.EMPTY;
-        {
-            synchronized( m_inventory )
-            {
-                return m_inventory.get( slot );
-            }
-        }
-        return ItemStack.EMPTY;
     }
 
     @Nonnull
     @Override
     public ItemStack removeStackFromSlot( int slot )
     {
-        synchronized( m_inventory )
+        ItemStack result = getStackInSlot( slot );
-        {
+        setInventorySlotContents( slot, ItemStack.EMPTY );
-            ItemStack result = getStackInSlot( slot );
+        return result;
-            setInventorySlotContents( slot, ItemStack.EMPTY );
-            return result;
-        }
     }
 
     @Nonnull
     @Override
     public ItemStack decrStackSize( int slot, int count )
     {
-        if( count == 0 )
+        if( count == 0 ) return ItemStack.EMPTY;
+
+        ItemStack stack = getStackInSlot( slot );
+        if( stack.isEmpty() ) return ItemStack.EMPTY;
+
+        if( stack.getCount() <= count )
         {
-            return ItemStack.EMPTY;
+            setInventorySlotContents( slot, ItemStack.EMPTY );
+            return stack;
         }
 
-        synchronized( m_inventory )
+        ItemStack part = stack.splitStack( count );
-        {
+        onInventoryDefinitelyChanged();
-            ItemStack stack = getStackInSlot( slot );
+        return part;
-            if( stack.isEmpty() )
-            {
-                return ItemStack.EMPTY;
-            }
-
-            if( stack.getCount() <= count )
-            {
-                setInventorySlotContents( slot, ItemStack.EMPTY );
-                return stack;
-            }
-
-            ItemStack part = stack.splitStack( count );
-            onInventoryDefinitelyChanged();
-            return part;
-        }
     }
 
     @Override
     public void setInventorySlotContents( int i, @Nonnull ItemStack stack )
     {
-        if( i >= 0 && i < INVENTORY_SIZE )
+        if( i >= 0 && i < INVENTORY_SIZE && !InventoryUtil.areItemsEqual( stack, m_inventory.get( i ) ) )
         {
-            synchronized( m_inventory )
+            m_inventory.set( i, stack );
-            {
+            onInventoryDefinitelyChanged();
-                if( !InventoryUtil.areItemsEqual( stack, m_inventory.get( i ) ) )
-                {
-                    m_inventory.set( i, stack );
-                    onInventoryDefinitelyChanged();
-                }
-            }
         }
     }
 
     @Override
     public void clear()
     {
-        synchronized( m_inventory )
+        boolean changed = false;
+        for( int i = 0; i < INVENTORY_SIZE; i++ )
         {
-            boolean changed = false;
+            if( !m_inventory.get( i ).isEmpty() )
-            for( int i = 0; i < INVENTORY_SIZE; i++ )
             {
-                if( !m_inventory.get( i ).isEmpty() )
+                m_inventory.set( i, ItemStack.EMPTY );
-                {
+                changed = true;
-                    m_inventory.set( i, ItemStack.EMPTY );
-                    changed = true;
-                }
-            }
-            if( changed )
-            {
-                onInventoryDefinitelyChanged();
             }
         }
+
+        if( changed ) onInventoryDefinitelyChanged();
     }
 
     @Override
     public void markDirty()
     {
         super.markDirty();
-        synchronized( m_inventory )
+        if( !m_inventoryChanged )
         {
-            if( !m_inventoryChanged )
+            for( int n = 0; n < getSizeInventory(); n++ )
             {
-                for( int n = 0; n < getSizeInventory(); n++ )
+                if( !ItemStack.areItemStacksEqual( getStackInSlot( n ), m_previousInventory.get( n ) ) )
                 {
-                    if( !ItemStack.areItemStacksEqual( getStackInSlot( n ), m_previousInventory.get( n ) ) )
+                    m_inventoryChanged = true;
-                    {
+                    break;
-                        m_inventoryChanged = true;
-                        break;
-                    }
                 }
             }
         }
@@ -574,8 +530,8 @@ private boolean hasPeripheralUpgradeOnSide( ComputerSide side )
     public void transferStateFrom( TileTurtle copy )
     {
         super.transferStateFrom( copy );
-        m_inventory = copy.m_inventory;
+        Collections.copy( m_inventory, copy.m_inventory );
-        m_previousInventory = copy.m_previousInventory;
+        Collections.copy( m_previousInventory, copy.m_previousInventory );
         m_inventoryChanged = copy.m_inventoryChanged;
         m_brain = copy.m_brain;
         m_brain.setOwner( this );