Skip to content

Commit

Permalink
Fix Memory Leak of Air Capability Listeners (#1275)
Browse files Browse the repository at this point in the history
* Invalidate and revive air capabilities

* Reuse air capability invalidation listeners

* Fix returning null instead of empty optional

* Check that the invalidated listener matches current

Also implement the cached listener for RegulatorModule as well.

* Add missing else branch to reset the cached air capabilities

* Split into getting cached and current neighbour air capabilities

* move tube module neighbour caching to separate interface

reduce code duplication

* Implement a cache for generic capabilities and neighbouring capabilities

* Stop creating a lambda on every cached capability get operation

---------

Co-authored-by: Des Herriott <[email protected]>
  • Loading branch information
BlueAgent and desht committed Dec 29, 2023
1 parent 3822681 commit 5c95d19
Show file tree
Hide file tree
Showing 9 changed files with 246 additions and 51 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
public abstract class AbstractAirHandlingBlockEntity extends AbstractTickingBlockEntity {
@GuiSynced
protected final IAirHandlerMachine airHandler;
private final LazyOptional<IAirHandlerMachine> airHandlerCap;
private LazyOptional<IAirHandlerMachine> airHandlerCap;
private final Map<IAirHandlerMachine, List<Direction>> airHandlerMap = new IdentityHashMap<>();

public AbstractAirHandlingBlockEntity(BlockEntityType<?> type, BlockPos pos, BlockState state, PressureTier pressureTier, int volume, int upgradeSlots) {
Expand All @@ -58,6 +58,19 @@ public AbstractAirHandlingBlockEntity(BlockEntityType<?> type, BlockPos pos, Blo
this.airHandlerCap = LazyOptional.of(() -> airHandler);
}

@Override
public void invalidateCaps() {
this.airHandlerCap.invalidate();
this.airHandlerCap = LazyOptional.empty();
super.invalidateCaps();
}

@Override
public void reviveCaps() {
super.reviveCaps();
this.airHandlerCap = LazyOptional.of(() -> airHandler);
}

@Override
public void handleUpdateTag(CompoundTag tag) {
super.handleUpdateTag(tag);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

public class TubeJunctionBlockEntity extends AbstractAirHandlingBlockEntity {
private final IAirHandlerMachine tube2Handler;
private final LazyOptional<IAirHandlerMachine> tube2Cap;
private LazyOptional<IAirHandlerMachine> tube2Cap;

public TubeJunctionBlockEntity(BlockPos pPos, BlockState pState) {
super(ModBlockEntities.TUBE_JUNCTION.get(), pPos, pState, PressureTier.TIER_TWO, 4000, 0);
Expand All @@ -28,6 +28,19 @@ public TubeJunctionBlockEntity(BlockPos pPos, BlockState pState) {
this.tube2Cap = LazyOptional.of(() -> tube2Handler);
}

@Override
public void invalidateCaps() {
this.tube2Cap.invalidate();
this.tube2Cap = LazyOptional.empty();
super.invalidateCaps();
}

@Override
public void reviveCaps() {
super.reviveCaps();
this.tube2Cap = LazyOptional.of(() -> tube2Handler);
}

@Override
public IItemHandler getPrimaryInventory() {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public class VacuumPumpBlockEntity extends AbstractAirHandlingBlockEntity implem
IRedstoneControl<VacuumPumpBlockEntity>, IManoMeasurable, MenuProvider {
@GuiSynced
private final IAirHandlerMachine vacuumHandler;
private final LazyOptional<IAirHandlerMachine> vacuumCap;
private LazyOptional<IAirHandlerMachine> vacuumCap;
public int rotation;
public int oldRotation;
private int turnTimer = -1;
Expand All @@ -71,6 +71,19 @@ public VacuumPumpBlockEntity(BlockPos pos, BlockState state) {
this.vacuumCap = LazyOptional.of(() -> vacuumHandler);
}

@Override
public void invalidateCaps() {
this.vacuumCap.invalidate();
this.vacuumCap = LazyOptional.empty();
super.invalidateCaps();
}

@Override
public void reviveCaps() {
super.reviveCaps();
this.vacuumCap = LazyOptional.of(() -> vacuumHandler);
}

@Nonnull
@Override
public <T> LazyOptional<T> getCapability(@Nonnull Capability<T> cap, @Nullable Direction side) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package me.desht.pneumaticcraft.common.capabilities;

import net.minecraftforge.common.util.LazyOptional;
import net.minecraftforge.common.util.NonNullConsumer;
import org.jetbrains.annotations.NotNull;

/**
* Generic capability cache.
* <br/>
* Typical usage is to {@link CapabilityCache#get()} and use it if it is present, otherwise call {@link CapabilityCache#set(LazyOptional)} to update the capability cache.
*/
public final class CapabilityCache<T> {
private LazyOptional<T> cachedCapability;
private final NonNullConsumer<LazyOptional<T>> capabilityInvalidationListener;

public CapabilityCache() {
this.cachedCapability = LazyOptional.empty();
this.capabilityInvalidationListener = l -> {
if (this.cachedCapability != l) {
return;
}

this.cachedCapability = LazyOptional.empty();
};
}

public @NotNull LazyOptional<T> get() {
return this.cachedCapability;
}

/**
* Sets the cached capability.
* Also handles registering an invalidation listener to clear the cached capability.
*
* @param cap Capability to set.
* @return The capability that was set.
*/
public @NotNull LazyOptional<T> set(LazyOptional<T> cap) {
if (this.cachedCapability == cap) {
return cap;
}

if (!cap.isPresent()) {
this.cachedCapability = LazyOptional.empty();
return this.cachedCapability;
}

this.cachedCapability = cap;
cap.addListener(this.capabilityInvalidationListener);
return this.cachedCapability;
}

public void clear() {
this.cachedCapability = LazyOptional.empty();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public class MachineAirHandler extends BasicAirHandler implements IAirHandlerMac
private Direction leakDir = null;
private Direction prevLeakDir = null;
private int prevAir;
private final Map<Direction, LazyOptional<IAirHandlerMachine>> neighbourAirHandlers = new EnumMap<>(Direction.class);
private final NeighbouringCapabilityCache<IAirHandlerMachine> neighbouringAirHandlerCache;

// note: leaks due to security upgrade are tracked separately from leaks due to disconnection
private boolean safetyLeaking; // is the handler venting right now?
Expand All @@ -69,9 +69,7 @@ public MachineAirHandler(PressureTier tier, int volume) {
super(volume);

this.tier = tier;
for (Direction dir : DirectionUtil.VALUES) {
this.neighbourAirHandlers.put(dir, LazyOptional.empty());
}
this.neighbouringAirHandlerCache = new NeighbouringCapabilityCache<>();
}

@Override
Expand Down Expand Up @@ -122,10 +120,7 @@ public void setConnectedFaces(List<Direction> sides) {
connectedFaces.clear();
sides.forEach(side -> connectedFaces.set(side.get3DDataValue()));

// invalidate cached neighbour data
for (Direction dir : DirectionUtil.VALUES) {
this.neighbourAirHandlers.put(dir, LazyOptional.empty());
}
this.neighbouringAirHandlerCache.clear();
}

@Override
Expand Down Expand Up @@ -240,22 +235,15 @@ public Direction getSideLeaking() {
return this.leakDir;
}

private LazyOptional<IAirHandlerMachine> getNeighbourAirHandler(BlockEntity ownerTE, Direction dir) {
private LazyOptional<IAirHandlerMachine> getCachedNeighbourAirHandler(BlockEntity ownerTE, Direction dir) {
if (!connectedFaces.get(dir.get3DDataValue())) return LazyOptional.empty();

if (!neighbourAirHandlers.get(dir).isPresent()) {
BlockEntity te1 = Objects.requireNonNull(ownerTE.getLevel()).getBlockEntity(ownerTE.getBlockPos().relative(dir));
if (te1 != null) {
LazyOptional<IAirHandlerMachine> cap = te1.getCapability(PNCCapabilities.AIR_HANDLER_MACHINE_CAPABILITY, dir.getOpposite());
if (cap.isPresent()) {
neighbourAirHandlers.put(dir, cap);
neighbourAirHandlers.get(dir).addListener(l -> neighbourAirHandlers.put(dir, LazyOptional.empty()));
}
} else {
neighbourAirHandlers.put(dir, LazyOptional.empty());
}
LazyOptional<IAirHandlerMachine> cap = this.neighbouringAirHandlerCache.get(dir);
if (cap.isPresent()) {
return cap;
}
return neighbourAirHandlers.get(dir);

return this.neighbouringAirHandlerCache.set(PNCCapabilities.AIR_HANDLER_MACHINE_CAPABILITY, ownerTE, dir);
}

private void disperseAir(BlockEntity ownerTE) {
Expand Down Expand Up @@ -292,7 +280,7 @@ private List<Connection> getConnectedAirHandlers(BlockEntity ownerTE, boolean on
List<IAirHandlerMachine.Connection> neighbours = new ArrayList<>();
for (Direction dir : DirectionUtil.VALUES) {
if (connectedFaces.get(dir.get3DDataValue())) {
getNeighbourAirHandler(ownerTE, dir).ifPresent(h -> {
getCachedNeighbourAirHandler(ownerTE, dir).ifPresent(h -> {
if ((!onlyLowerPressure || h.getPressure() < getPressure())) {
neighbours.add(new ConnectedAirHandler(dir, h));
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package me.desht.pneumaticcraft.common.capabilities;

import me.desht.pneumaticcraft.common.util.CapabilityUtils;
import net.minecraft.core.Direction;
import net.minecraft.world.level.block.entity.BlockEntity;
import net.minecraftforge.common.capabilities.Capability;
import net.minecraftforge.common.util.LazyOptional;
import org.jetbrains.annotations.NotNull;

import java.util.EnumMap;
import java.util.Map;

/**
* Capability cache for neighbours of a {@link BlockEntity}.
* <br/>
* Typical usage is to {@link NeighbouringCapabilityCache#get(Direction)} and use it if it is present,
* otherwise call {@link NeighbouringCapabilityCache#set(Capability, BlockEntity, Direction)} to update the capability cache.
*/
public class NeighbouringCapabilityCache<T> {
private final Map<Direction, CapabilityCache<T>> neighbouringCapabilityCaches;

public NeighbouringCapabilityCache() {
this.neighbouringCapabilityCaches = new EnumMap<>(Direction.class);
}

/**
* Get cached capability for a neighbouring {@link BlockEntity}.
*
* @param direction Direction from the original block entity to a neighbouring block entity.
* @return The cached capability.
*/
public @NotNull LazyOptional<T> get(@NotNull Direction direction) {
CapabilityCache<T> cache = this.neighbouringCapabilityCaches.get(direction);
if (cache == null) {
return LazyOptional.empty();
}

return cache.get();
}

/**
* Sets the cached capability for a neighbouring {@link BlockEntity}.
* Also handles registering an invalidation listener to clear the cached capability.
*
* @param capability Capability to get and cache.
* @param blockEntity Originating {@link BlockEntity}.
* @param direction Direction from {@param blockEntity} to a neighbouring block entity to look for a capability.
* @return The cached capability.
*/
public @NotNull LazyOptional<T> set(@NotNull Capability<T> capability, @NotNull BlockEntity blockEntity, @NotNull Direction direction) {
CapabilityCache<T> cache = this.neighbouringCapabilityCaches.get(direction);
if (cache == null) {
cache = new CapabilityCache<>();
this.neighbouringCapabilityCaches.put(direction, cache);
}

return cache.set(CapabilityUtils.getNeighbourCap(capability, blockEntity, direction));
}

public void clear() {
for (Direction dir : Direction.values()) {
clear(dir);
}
}

public void clear(@NotNull Direction dir) {
CapabilityCache<T> cache = this.neighbouringCapabilityCaches.get(dir);
if (cache == null) {
return;
}

cache.clear();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,21 @@
import me.desht.pneumaticcraft.api.tileentity.IAirHandlerMachine;
import me.desht.pneumaticcraft.common.block.PressureTubeBlock;
import me.desht.pneumaticcraft.common.block.entity.PressureTubeBlockEntity;
import me.desht.pneumaticcraft.common.capabilities.CapabilityCache;
import me.desht.pneumaticcraft.common.core.ModItems;
import me.desht.pneumaticcraft.common.util.CapabilityUtils;
import me.desht.pneumaticcraft.lib.PneumaticValues;
import net.minecraft.core.Direction;
import net.minecraft.world.item.Item;
import net.minecraft.world.level.block.entity.BlockEntity;
import net.minecraftforge.common.util.LazyOptional;

public class RegulatorModule extends AbstractRedstoneReceivingModule implements IInfluenceDispersing {
private LazyOptional<IAirHandlerMachine> neighbourCap = null;
private final CapabilityCache<IAirHandlerMachine> neighbourAirHandlerCache;

public RegulatorModule(Direction dir, PressureTubeBlockEntity pressureTube) {
super(dir, pressureTube);

this.neighbourAirHandlerCache = new CapabilityCache<>();
}

@Override
Expand Down Expand Up @@ -60,7 +63,8 @@ public boolean isInline() {
@Override
public void onNeighborBlockUpdate() {
super.onNeighborBlockUpdate();
neighbourCap = null;

this.neighbourAirHandlerCache.clear();
}

@Override
Expand All @@ -70,16 +74,12 @@ public float getThreshold() {
}

private LazyOptional<IAirHandlerMachine> getCachedNeighbourAirHandler() {
if (neighbourCap == null) {
BlockEntity neighborTE = pressureTube.nonNullLevel().getBlockEntity(pressureTube.getBlockPos().relative(dir));
if (neighborTE != null) {
neighbourCap = neighborTE.getCapability(PNCCapabilities.AIR_HANDLER_MACHINE_CAPABILITY, dir.getOpposite());
if (neighbourCap.isPresent()) neighbourCap.addListener(l -> neighbourCap = null);
} else {
neighbourCap = LazyOptional.empty();
}
LazyOptional<IAirHandlerMachine> cap = this.neighbourAirHandlerCache.get();
if (cap.isPresent()) {
return cap;
}
return neighbourCap;

return this.neighbourAirHandlerCache.set(CapabilityUtils.getNeighbourCap(PNCCapabilities.AIR_HANDLER_MACHINE_CAPABILITY, pressureTube, dir));
}

@Override
Expand Down
Loading

0 comments on commit 5c95d19

Please sign in to comment.