[Solved] [1.16.5] Issue to do with threads that break or "freeze" the server side without error

[Astro2202]

There is a specific scenario where the game breaks but doesn't crash and doesn't give me a crash report.
My mod is supposed to replace certain blocks after a certain amount of time in the world. This already works.

Quick explanation: It's a decay mod where blocks can break and decay until they are completely gone. Only blocks placed by the player are affected.

I implemented it in to a capability in order to save which blocks need to change after the player would quit the world. So the decaying and the saving of which blocks need to decay already work but here's the problem.
When I quit the game and restart it works.
When I save and quit to title and then reload the world, the issue occurs.
During this scenario as soon as the block is supposed to decay, everything stops working without explanation.
I can see in the console that a block is about to decay and then nothing...

This is the basic class in which I worked out the concept of decay on just stone bricks:

public class DefaultDecay implements IDecay {

    private List<BlockPos> blockPosList;
    private List<BlockPos> blocksToBeRemoved;
    private World world;
    private Random random;
    private int decayChance;

    public DefaultDecay(){
        this.blockPosList = new ArrayList<>();
        this.blocksToBeRemoved = new ArrayList<>();
        this.random = new Random();
        this.decayChance = 1000;
        MinecraftForge.EVENT_BUS.register(this);
    }

    @Override
    public List<BlockPos> getAllBlockPositions() {
        return this.blockPosList;
    }

    @Override
    public void addBlock(BlockPos blockPos) {
        blockPosList.add(blockPos);
    }

    @Override
    public void removeBlock(BlockPos blockPosToRemove) {
        BlockPos tempBlockPos = BlockPos.ZERO;
        for(BlockPos blockPos : blockPosList){
            if(blockPos.equals(blockPosToRemove)){
                tempBlockPos = blockPos;
            }
        }
        if(!tempBlockPos.equals(BlockPos.ZERO)){
            blockPosList.remove(tempBlockPos);
            System.out.println("Decay logic on position removed");
        }
    }

    @SubscribeEvent
    public void onWorldTick(final TickEvent.WorldTickEvent event){
        if(this.world == null){
            this.world = event.world;
        }
        if(random.nextInt(2) == 0){
            swapBlock();
        }
    }

    public void swapBlock(){
        if(!world.isClientSide){
            for(BlockPos blockPos : blockPosList){
               if(random.nextInt(decayChance) == 0){
                    System.out.println(blockPos + " is Decaying");
                    System.out.println(world);
                    if(world.setBlockAndUpdate(blockPos, Blocks.CRACKED_STONE_BRICKS.getBlock().defaultBlockState())){ //TODO: Fix issue when reloading world causing server freeze
                        this.blocksToBeRemoved.add(blockPos);
                        System.out.println(blockPos + " Decayed");
                    }
                }
            }
        }
        blockPosList.removeAll(blocksToBeRemoved);
        blocksToBeRemoved.clear();
    }
}

A lot of code will be changed, this is just to see if I can make it work.
When the problem occurs in that specific scenario that I described above it happens here:

if(world.setBlockAndUpdate(blockPos, Blocks.CRACKED_STONE_BRICKS.getBlock().defaultBlockState()))

I already checked with debugging.  the block position is correct and the world object is correct. When the code steps in the setBlockAndUpdate method, the problem occurs and the code is interrupted without an error message or crash.
I traced the steps going in to this method when it works and when it doesn't. Here's where the first difference is:

 @Nullable
   public IChunk getChunk(int p_212849_1_, int p_212849_2_, ChunkStatus p_212849_3_, boolean p_212849_4_) {
      if (Thread.currentThread() != this.mainThread) {
         return CompletableFuture.supplyAsync(() -> {
            return this.getChunk(p_212849_1_, p_212849_2_, p_212849_3_, p_212849_4_);
         }, this.mainThreadProcessor).join();
      } else {
         IProfiler iprofiler = this.level.getProfiler();
         iprofiler.incrementCounter("getChunk");
         long i = ChunkPos.asLong(p_212849_1_, p_212849_2_);
			....

When it works, the if statement is false. But when the world is reloaded without quitting the game it's true. So it has to do with threads. When the .join() method is called I'm no longer able to understand what's going on. What I can tell you is that it goes down a couple of steps more before the thread is "parked". Don't quote me on that though I hardly understand it. When this happens, the game doesn't crash but mobs don't walk, commands don't work, blocks don't update etc... The server side freezes.

I already had a topic post on this previously but a lot of my code has changed and now I have more information about the issue. The previous post was too vague and got lengthy so I'm hoping that with the start of this post I can be more clear about the issue at hand.
Here is a link to the previous post: 

It might contain some code, context or information that can be useful.

Here is a link to all the source code of the project available on GitHub: https://github.com/Astro2202/DecayMod
I'm using Intellij.

This is a pretty complicated issue. There is not necessarily anything wrong about my code. I'm just wondering if anyone has a clue of what I can do to prevent this from happening and might have an explanation for why this is happening.

Edited May 26, 2021 by Astro2202

[SerpentDagger]

    @SubscribeEvent
    public void onWorldTick(final TickEvent.WorldTickEvent event){
        if(this.world == null){
            this.world = event.world;
        }
        if(random.nextInt(2) == 0){
            swapBlock();
        }
    }

My guess would be that this is the problem. You only ever set the World object once, and I don't think it's the same object between loads of the world, which means that you're trying to edit the old World when it is no longer being processed. It seems likely that yours is the last reference to that World object, since it should have been unloaded, which would also be causing a memory leak.

From the previous topic, this is a Capability, right? That means you can unload the IDecay object during the write/readNBT methods. It looks like you're applying the decay logic every tick, which means that it should be feasible to discard the IDecay objects between world-loadings.

[Astro2202]

11 hours ago, SerpentDagger said:

    @SubscribeEvent
    public void onWorldTick(final TickEvent.WorldTickEvent event){
        if(this.world == null){
            this.world = event.world;
        }
        if(random.nextInt(2) == 0){
            swapBlock();
        }
    }

My guess would be that this is the problem. You only ever set the World object once, and I don't think it's the same object between loads of the world, which means that you're trying to edit the old World when it is no longer being processed. It seems likely that yours is the last reference to that World object, since it should have been unloaded, which would also be causing a memory leak.

From the previous topic, this is a Capability, right? That means you can unload the IDecay object during the write/readNBT methods. It looks like you're applying the decay logic every tick, which means that it should be feasible to discard the IDecay objects between world-loadings.

So just to make sure that I get this right, the IDecay objects are not unloaded when I unload the world? I just put this "System.out.println("Initialized DefaultDecay");" in the constructor of DefaultDecay to see when it would be initialized and how many times. I don't fully understand why but when I load the world for the first time it's printed three times. When I quit to title and reload the world it's also printed three times. Doesn't this mean it's reinitialized? I'm just not sure if the IDecay object is actually unloaded or not when I reload the world.

Edit: I already figured out that this event is called three times:
 

@SubscribeEvent
    public static void onAttachingCapabilitiesEvent(final AttachCapabilitiesEvent<World> event){
        if(event.getObject() instanceof World){
            DecayProvider provider = new DecayProvider();
            event.addCapability(new ResourceLocation(DecayMod.MOD_ID, "decayhandler"), provider);
            event.addListener(provider::invalidate);
            System.out.println("Capability attached");
        }
    }

As you can see I also did a console print here and Capability attached is also printed three times every time I load in to the world. Doesn't this mean that an IDecay object is newly created every time I load in to the world? Three times apparently I don't know what that is about. 

Edited May 25, 2021 by Astro2202

[Astro2202]

22 hours ago, diesieben07 said:

Yes they are unloaded. But you are storing a hard reference to the World object, which prevents it from being garbage collected and also means you have an invalid reference now.

Don't store the world object in a fields.

Thank you for your advice!
Instead of storing a hard reference I just use the current world object of the event every time to try and replace the block in:
 

@SubscribeEvent
    public void onWorldTick(final TickEvent.WorldTickEvent event){
        swapBlock(event.world);
    }

    public void swapBlock(World world){
        if(!world.isClientSide){
            for(BlockPos blockPos : blockPosList){
               if(random.nextInt(decayChance) == 0){
                    System.out.println(blockPos + " is Decaying");
                    //System.out.println(world);
                    if(world.setBlockAndUpdate(blockPos, Blocks.CRACKED_STONE_BRICKS.getBlock().defaultBlockState())){ //TODO: Fix issue when reloading world causing server freeze
                        this.blocksToBeRemoved.add(blockPos);
                        System.out.println(blockPos + " Decayed");
                    }
                }
            }
        }
        blockPosList.removeAll(blocksToBeRemoved);
        blocksToBeRemoved.clear();
    }

This has changed all behavior and I'm not fully sure of what's going on.
I'll have to explain this step by step because it's pretty specific.

When I first load the world from first startup of the game, I place a couple of stone bricks.
Some stone bricks change in to cracked stone bricks and the console mentioned they decayed. first weird thing is that sometimes the console says blockpos is decaying and blockpos decayed twice about the same block.
Some stone bricks don't seem to decay at all. The console often mentions that a block is decaying and has decayed without the block changing in the game. Because of this they are removed out of the blockpos list and decay logic is no longer applied to them but they haven't actually been replaced with cracked stone bricks ingame.
The console also prints often that a block is decaying but not that it has decayed.

When I now save and quit to title and reload the world as well as restart the game the same behavior persists for as far as I can tell.

My guess is that it either has to do with that the capability is attached three times or the world object is not consistent.

I added a print line in the AttachingCapabilitiesEvent:
 

@SubscribeEvent
    public static void onAttachingCapabilitiesEvent(final AttachCapabilitiesEvent<World> event){
        if(event.getObject() instanceof World){
            DecayProvider provider = new DecayProvider();
            event.addCapability(new ResourceLocation(DecayMod.MOD_ID, "decayhandler"), provider);
            event.addListener(provider::invalidate);
            System.out.println("Capability attached");
        }
    }

And the console always prints "Capability attached" three times when I load in to a world. Perhaps there are multiple IDecay objects and that's causing this behavior but I'm just guessing at this point.

[Astro2202]

5 hours ago, diesieben07 said:

There is more than one world (overworld, end, nether as well as any potential modded dimensions) and all of them will tick. So you can't just have a global list of positions, because they will be shared across all worlds. This is what your capability is for, it needs to store the positions for that world.

Also note that tick events fire twice every tick, you have to check the phase field.

So as a temporary solution I added a check to see if the dimension is the overworld and will only swap the block on the end phase:
 

@SubscribeEvent
    public void onWorldTick(final TickEvent.WorldTickEvent event){
        if(event.phase.equals(TickEvent.Phase.END)){
            if(event.world.dimension().equals(World.OVERWORLD)){
                swapBlock(event.world);
            }
        }

    }

    public void swapBlock(World world){
        if(!world.isClientSide){
            for(BlockPos blockPos : blockPosList){
               if(random.nextInt(decayChance) == 0){
                    System.out.println(blockPos + " is Decaying");
                    //System.out.println(world);
                    if(world.setBlockAndUpdate(blockPos, Blocks.CRACKED_STONE_BRICKS.getBlock().defaultBlockState())){ //TODO: Fix issue when reloading world causing server freeze
                        this.blocksToBeRemoved.add(blockPos);
                        System.out.println(blockPos + " Decayed");
                    }
                }
            }
        }
        blockPosList.removeAll(blocksToBeRemoved);
        blocksToBeRemoved.clear();
    }

This somewhat fixed it in the sense that all blocks that are placed now decay. It also just works when I reload the world or reload the game entirely.

But another problem still remains..
All the block positions are stored in blockPosList and when a block decays it should be removed from this list.
Somehow certain blocks don't get removed from this list or even reappear in the list after being removed. I tried to track this behavior with debug but can't find where or why these positions keep being added to the list.

Because of this, some blocks that are already cracked keep getting called to be decayed. Though when it does, the setBlockAndUpdate method returns false and never updates (maybe because it already is a cracked stone brick?).
When I remove the block ingame, the position still remains and because of this a cracked stone brick block appears out of thin air in the position where I just broke the block. Even when this happens and I remove the block again, the position remains in the list

the list is obviously private and new positions can only be added with the addBlock method. This method is only called in the BlockEvent.EntityPlaceEvent event and in the capability itself when loading the nbt data which puts all the saved positions back in to the IDecay instance.
For further troubleshooting I also print what I save and what I load in the capability. Even though the list still contains these positions that shouldn't be there, it seems like they are not always saved when pressing escape. When pressing escape I can clearly see in the console that the world is being saved and there is no blockposition that is being saved with it. That said this isn't always the case. in some cases there is a blockpos that is still being saved but the confusing part is that this doesn't always happen and when it does there are also other blockpositions in the list that are not saved with it. It's very confusing and I've been trying to pinpoint where the bug is but failed so far. My guess is that it still has something to do with the different dimensions although it always clearly says overworld and in the other dimensions nothing is ever saved.

So the blocks that have decayed somehow don't get removed or reappear in the list. and when these positions are in the list they may or may not be saved with the capability.
When I debug I can see that if a block successfully decays it's added to blocksToBeRemoved and in turn will be removed from blockPosList. This works. I can see that when the removeAll method is ran, the blockPosList shrinks with the amount that blocksToBeRemoved was. It's when the game keeps running that out of nowhere blockPosList contains the blockpositions again. I thought maybe they are loaded back in from the capability but I can't see the load happening in the console. It's at this point that I wanted to see what would be saved when I press escape and it's here that I noticed these positions that magically appeared again in the list are not being saved (most of them).
I'm sorry if this is a complicating explanation.

Edited May 26, 2021 by Astro2202

[Astro2202]

1 minute ago, diesieben07 said:

Please post a Git repo of your mod.

https://github.com/Astro2202/DecayMod

Edit: I'm not even sure what triggers this behavior. What I do is place blocks and wait for some of them to decay. I remove a block that hasn't decayed and a block that has. Then I sometimes reload the world and break another decayed and non-decayed block.

Edited May 26, 2021 by Astro2202

[Astro2202]

1 hour ago, diesieben07 said:

Your repository should not contain the run, build or .gradle folder (note: .gradle, gradle should be included). The MDK already comes with a properly configured .gitignore file. Please use it.

I don't think I made it work yet but I'm on it.

I removed both EventHandler and WorldDecayData. It was some unused leftover code from previous attempts.

 

1 hour ago, diesieben07 said:

This method is just bizarre. As far as I can tell all this accomplishes is just blockPosList.remove(pos). blockPosList should also almost definitely be a Set and not a List.

Fixed!

 

1 hour ago, diesieben07 said:

Because you attach a DecayProvider to each world, you are creating a new DefaultDecay instance for each world. This instance then registers itself to the event bus. This registration is never removed and thus your DefaultDecay instance will keep receiving events even after it is long invalid (due to its world being unloaded). Avoid dynamic event listeners like this.

I put the worldTickEvent in the EventHandler class and made it call a new method onTick() in DefaultDecay. I think this should fix that issue.
 

 

1 hour ago, diesieben07 said:

This code is highly inefficient.

• Why is blocksToBeRemoved a field?
• Why are you not using an iterator and Iterator#remove? Then you don't need the "to be removed" list at all.

I implemented the iterator instead and you are right this is much much better.

 

1 hour ago, diesieben07 said:

Your DecayProvider#getCapability method is invalid. You can't just blindly return your capability. You must check if it is actually the capability being queried for.

Can you further elaborate on this? This is the part of the capability which I'm not fully sure of how it works.
 

 

1 hour ago, diesieben07 said:

• This can cause a NullPointerException as getEntity may return null.
• This should use the RegistryKey constants in World, not in Dimension.

And both these are no longer an issue.

I am pleased to say that all functions well now! I'm no longer experiencing any major bugs.
Thank you very much for this useful advice. I know I made some real basic mistakes here but now I know better and I will be a better programmer/modder because of it. So once again I'm very grateful, thank you!

All the changes I made so far are pushed on to GitHub.

[Astro2202]

15 hours ago, diesieben07 said:

The getCapability method has a capability parameter. This is the capability being asked for. If it is not your capability, you need to return the empty optional to indicate that you do not have that capability.

Like this?
 

@Nonnull
    @Override
    public <T> LazyOptional<T> getCapability(@Nonnull Capability<T> cap, @Nullable Direction side) {
        if(cap.getName() != DecayCapability.DECAY_CAPABILITY.getName()){
            return LazyOptional.empty();
        }
        else{
            return decayHandlerOptional.cast();
        }
    }

 

[Astro2202]

3 minutes ago, diesieben07 said:

Just compare the Capability, not its name.

Got it.
 

if(cap.equals(DecayCapability.DECAY_CAPABILITY)){
  return decayHandlerOptional.cast();
}
else{
  return LazyOptional.empty();
}

Thank you very much for all your help! It's been incredibly useful.