Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bugfix/#758 concurrency issue #1025

Merged
merged 3 commits into from
May 21, 2017
Merged

Bugfix/#758 concurrency issue #1025

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b88f955
Centralize the copying of the needed items list, and make it explicit…
cltnschlosser May 20, 2017
fc5dc50
Remove duplicated methods, and unnecessary casts
cltnschlosser May 20, 2017
28b251e
Change CopyOnWriteArrayList to ConcurrentLinkedQueue for current ware…
cltnschlosser May 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/main/java/com/minecolonies/coremod/colony/buildings/AbstractBuilding.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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())
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR This call to "contains()" may be a performance hot spot if the collection is large. rule

&& (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))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR This call to "contains()" may be a performance hot spot if the collection is large. rule

{
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))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR This call to "contains()" may be a performance hot spot if the collection is large. rule

{
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))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR This call to "contains()" may be a performance hot spot if the collection is large. rule

{
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