Skip to content

Commit

Permalink
Bugfix/#758 concurrency issue (#1025)
Browse files Browse the repository at this point in the history 
* Centralize the copying of the needed items list, and make it explicitly clear a copy is happening

* Remove duplicated methods, and unnecessary casts

* Change CopyOnWriteArrayList to ConcurrentLinkedQueue for current warehouse tasks.

The list is being modified often, and being treated like a queue. The ConcurrentLinkedQueue should be a better data structure for the situation and be more performant.
  • Loading branch information
@cltnschlosser @Raycoms
cltnschlosser authored and Raycoms committed May 21, 2017
1 parent 6445381 commit e9714d4
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 95 deletions.
11 changes: 7 additions & 4 deletions src/main/java/com/minecolonies/coremod/colony/buildings/AbstractBuilding.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1054,13 +1054,16 @@ public void addNeededItems(@Nullable ItemStack stack)
}

/**
* Getter for the neededItems.
* This method makes a copy of the itemsCurrentlyNeeded.
* Currently every call to this method needs a copy, if for some reason an outside
* class needs this list and a copy isn't desired, a new method should be created,
* and this doc should be changed to point to that new method.
*
* @return an unmodifiable list.
* @return a copy of the itemsCurrentlyNeeded list.
*/
public List<ItemStack> getNeededItems()
public List<ItemStack> getCopyOfNeededItems()
{
return Collections.unmodifiableList(itemsCurrentlyNeeded);
return new ArrayList<>(itemsCurrentlyNeeded);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion .../java/com/minecolonies/coremod/entity/ai/citizen/deliveryman/EntityAIWorkDeliveryman.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ private AIState prepareDelivery()
buildingToDeliver.setOnGoingDelivery(false);
return START_WORKING;
}
itemsToDeliver = new ArrayList<>(buildingToDeliver.getNeededItems());
itemsToDeliver = buildingToDeliver.getCopyOfNeededItems();
return GATHER_IN_WAREHOUSE;
}
}
Expand Down
121 changes: 31 additions & 90 deletions src/main/java/com/minecolonies/coremod/tileentities/TileEntityWareHouse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@
import org.jetbrains.annotations.Nullable;

import java.util.Map;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.Queue;
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.function.Predicate;

/**
Expand All @@ -28,9 +29,9 @@
public class TileEntityWareHouse extends TileEntityColonyBuilding
{
/**
* List which contains the currentTasks to be executed by the deliveryman.
* Queue which contains the currentTasks to be executed by the deliveryman.
*/
private final CopyOnWriteArrayList<AbstractBuilding> list = new CopyOnWriteArrayList<>();
private final Queue<AbstractBuilding> taskQueue = new ConcurrentLinkedQueue<>();

/**
* Wait this amount of ticks before checking again.
Expand Down Expand Up @@ -92,10 +93,10 @@ public void update()
if (i == index)
{
if(buildingEntry.getValue() instanceof AbstractBuildingWorker
&& !list.contains(buildingEntry.getValue())
&& ((AbstractBuildingWorker) buildingEntry.getValue()).needsAnything())
&& !taskQueue.contains(buildingEntry.getValue())
&& (buildingEntry.getValue()).needsAnything())
{
checkInWareHouse((AbstractBuildingWorker) buildingEntry.getValue(), true);
checkInWareHouse(buildingEntry.getValue(), true);
}
else if(buildingEntry.getValue() instanceof BuildingHome && ((BuildingHome) buildingEntry.getValue()).isFoodNeeded())
{
Expand All @@ -117,32 +118,28 @@ public boolean checkInWareHouseForFood(final BuildingHome buildingEntry, final b
if (addToList)
{
buildingEntry.setOnGoingDelivery(true);
list.add(buildingEntry);
taskQueue.add(buildingEntry);
}
return true;
}

if (list.contains(buildingEntry))
if (taskQueue.contains(buildingEntry))
{
list.remove(buildingEntry);
taskQueue.remove(buildingEntry);
buildingEntry.setOnGoingDelivery(false);
}
}
return false;
}

/**
* Get the first task in the list.
* Get the first task in the taskQueue, or null if its empty.
* @return the building which needs a delivery.
*/
@Nullable
public AbstractBuilding getTask()
{
if(list.isEmpty())
{
return null;
}
return list.remove(0);
return taskQueue.poll();
}

/**
Expand All @@ -155,7 +152,7 @@ public boolean checkInWareHouse(@NotNull final AbstractBuilding buildingEntry, b
{
if(buildingEntry.areItemsNeeded())
{
for(final ItemStack stack : buildingEntry.getNeededItems())
for(final ItemStack stack : buildingEntry.getCopyOfNeededItems())
{
if(stack == null
|| (deliveryManHasBuildingAsTask(buildingEntry)
Expand All @@ -169,14 +166,15 @@ public boolean checkInWareHouse(@NotNull final AbstractBuilding buildingEntry, b
if(addToList)
{
buildingEntry.setOnGoingDelivery(true);
list.add(buildingEntry);
taskQueue.add(buildingEntry);
}
return true;
}
}
if (list.contains(buildingEntry))

if (taskQueue.contains(buildingEntry))
{
list.remove(buildingEntry);
taskQueue.remove(buildingEntry);
buildingEntry.setOnGoingDelivery(false);
}
}
Expand All @@ -189,13 +187,14 @@ public boolean checkInWareHouse(@NotNull final AbstractBuilding buildingEntry, b
if (addToList)
{
buildingEntry.setOnGoingDelivery(true);
list.add(buildingEntry);
taskQueue.add(buildingEntry);
}
return true;
}
if (list.contains(buildingEntry))

if (taskQueue.contains(buildingEntry))
{
list.remove(buildingEntry);
taskQueue.remove(buildingEntry);
buildingEntry.setOnGoingDelivery(false);
}
}
Expand Down Expand Up @@ -234,35 +233,17 @@ private boolean deliveryManHasBuildingAsTask(@NotNull final AbstractBuilding bui
* @param is the type of item requested (amount is ignored)
* @return true if a stack of that type was found
*/
public boolean isInHut(@Nullable final ItemStack is)
private boolean isInHut(@Nullable final ItemStack is)
{
@Nullable final AbstractBuilding building = getBuilding();
if(building != null)
{
if(isInTileEntity(building.getTileEntity(), is))
{
return true;
}

for(final BlockPos pos : building.getAdditionalCountainers())
{
@Nullable final TileEntity entity = world.getTileEntity(pos);
if(entity instanceof TileEntityChest && isInTileEntity((TileEntityChest) entity, is))
{
return true;
}
}
}

return false;
return is != null && isInHut(stack -> stack != null && is.isItemEqual(stack));
}

/**
* Check all chests in the worker hut for a required item.
* @param itemStackSelectionPredicate the type of item requested (amount is ignored).
* @return true if a stack of that type was found
*/
public boolean isInHut(@NotNull final Predicate<ItemStack> itemStackSelectionPredicate)
private boolean isInHut(@NotNull final Predicate<ItemStack> itemStackSelectionPredicate)
{
@Nullable final AbstractBuilding building = getBuilding();
if(building != null)
Expand All @@ -287,31 +268,13 @@ public boolean isInHut(@NotNull final Predicate<ItemStack> itemStackSelectionPre

/**
* Check for a certain item and return the position of the chest containing it.
* @param stack the stack to search for.
* @param is the stack to search for.
* @return the position or null.
*/
@Nullable
public BlockPos getPositionOfChestWithItemStack(@NotNull final ItemStack stack)
public BlockPos getPositionOfChestWithItemStack(@NotNull final ItemStack is)
{
@Nullable final AbstractBuilding building = getBuilding();

if(building != null)
{
if(isInTileEntity(building.getTileEntity(), stack))
{
return building.getLocation();
}

for(final BlockPos pos : building.getAdditionalCountainers())
{
final TileEntity entity = world.getTileEntity(pos);
if(entity instanceof TileEntityChest && isInTileEntity((TileEntityChest) entity, stack))
{
return pos;
}
}
}
return null;
return getPositionOfChestWithItemStack(stack -> stack != null && is.isItemEqual(stack));
}

/**
Expand Down Expand Up @@ -368,7 +331,7 @@ public BlockPos getPositionOfChestWithTool(@NotNull final String tool,final int
final TileEntity entity = world.getTileEntity(pos);
if (entity instanceof TileEntityChest
&& ((minLevel != -1 && InventoryUtils.isPickaxeInProvider(entity, minLevel, requestingBuilding.getBuildingLevel()))
|| InventoryUtils.isToolInProvider((TileEntityChest) entity, tool, requestingBuilding.getBuildingLevel())))
|| InventoryUtils.isToolInProvider(entity, tool, requestingBuilding.getBuildingLevel())))
{
return pos;
}
Expand All @@ -383,7 +346,7 @@ public BlockPos getPositionOfChestWithTool(@NotNull final String tool,final int
* @param requestingBuilding the building requesting it.
* @return true if a stack of that type was found
*/
public boolean isToolInHut(final String tool, @NotNull final AbstractBuilding requestingBuilding)
private boolean isToolInHut(final String tool, @NotNull final AbstractBuilding requestingBuilding)
{
@Nullable final AbstractBuilding building = getBuilding();

Expand Down Expand Up @@ -429,26 +392,6 @@ public boolean isToolInHut(final String tool, @NotNull final AbstractBuilding re
return false;
}

/**
* Finds the first @see ItemStack the type of {@code is}.
* It will be taken from the chest and placed in the workers inventory.
* Make sure that the worker stands next the chest to not break immersion.
* Also make sure to have inventory space for the stack.
* @param entity the tileEntity chest or building.
* @param is the itemStack.
* @return true if found the stack.
*/
public boolean isInTileEntity(TileEntityChest entity, ItemStack is)
{
return is != null
&& InventoryFunctions
.matchFirstInProviderWithAction(
entity,
stack -> stack != null && is.isItemEqual(stack),
InventoryFunctions::doNothing
);
}

/**
* Finds the first @see ItemStack the type of {@code is}.
* It will be taken from the chest and placed in the workers inventory.
Expand All @@ -458,14 +401,13 @@ public boolean isInTileEntity(TileEntityChest entity, ItemStack is)
* @param itemStackSelectionPredicate the itemStack predicate.
* @return true if found the stack.
*/
public boolean isInTileEntity(final TileEntityChest entity, @NotNull final Predicate<ItemStack> itemStackSelectionPredicate)
private boolean isInTileEntity(final TileEntityChest entity, @NotNull final Predicate<ItemStack> itemStackSelectionPredicate)
{
return InventoryFunctions
.matchFirstInProviderWithAction(
entity,
itemStackSelectionPredicate,
InventoryFunctions::doNothing
);
InventoryFunctions::doNothing);
}

@Override
Expand Down Expand Up @@ -504,7 +446,6 @@ public void dumpInventoryIntoWareHouse(@NotNull final InventoryCitizen inventory
}
InventoryUtils.transferItemStackIntoNextFreeSlotInProvider(new InvWrapper(inventoryCitizen), i, chest);
}

}

/**
Expand Down

0 comments on commit e9714d4

Please sign in to comment.