CC: Tweaked

CC: Tweaked

42M Downloads

Server crashes when turtle places an advanced monitor to an already existing monitor array

spr2-dev opened this issue ยท 3 comments

commented

System info

MC version : 1.16.4
Forge version : 35.1.29
CC:T version : 1.95.2

Description

In singleplayer, game crashes when a turtle is attempting to place an advanced monitor to an already existing monitor array.
This is the setup I used (turtle places monitor on the top-right hand corner of the array)
image
According to the crash dump, the issue is server-side, happened on monitor expansion, and seems to be a race condition (oh no).

Repro steps

Log into your singleplayer world,
Place the monitors as per the screenshot above,
Make sure you place a block (type doesn't matter, solid by preference) 2 blocks in front of the turtle, so the monitor screen faces the right direction (faces the turtle).
Make the turtle place the top-right monitor by adding one in its active slot, and calling turtle.place().

Additional info

The bug was originally discovered by xXTurnerLP, and I managed to reproduce it. Same MC and CC:T versions were used.

commented

Similar bugs happen on 1.15, and boy this is fun. The TLDR is that it's a vanilla bug, which I'm sure we'll have to work around in weird and wonderful ways.

The rough code path is as followed:

  • We start ticking the turtle, and prepare to place a block.
  • The monitor is placed.
  • When placed, we attempt to merge ourselves into other monitors. This involves updating the block state of the current block.

So far, so good. The issue is that we're doing all of this while the turtle is being ticked.

As a bit of background, if you add/remove tile entities while another TE is ticking, they get added to the pendingBlockEntities list rather than being set in-world (in order to avoid CMEs on the main blockEntityList list). When you call World.getBlockEntity(BlockPos), Minecraft is smart enough to check this list of tiles. Well, sometimes at least.

Monitors call World.setBlock to update their state with the new adjacent information. This then calls Chunk.setBlockState to actually set the block. Chunk.setBlockState sets the block, and then checks if this block needs a tile. If such a tile isn't in the world, it creates a new one and sets it.

"such a tile isn't in the world" is the problem here. Chunk consults its own internal store (rather than the World one), which means it entirely ignores our pendingBlockEntities list! And so it think there's no tile there, removes the one which is actually there and creates a new one.


Yeah, I don't know what to do about that. The obvious solution is a patch to Forge to fix this issue, but I don't know if there's a way of fixing this efficiently.

We should probably report this to Mojang too, though it only really surfaces under modded, which probably makes it harder to find an actual test case.

Alternative solution is to detect if check if World.updatingBlockEntities is true and just schedule the update for the next tick. It's an ugly work around, but it'd work I guess.

commented

Delaying the update to next tick could solve the issue, but as you said it would be ugly and pretty bad to debug if there are other issues with it.

I didn't document that in this thread, but there are other issues with turtles placing monitors I discovered while trying to replicate. One of them is that game doesn't crash, but the black part of the monitor (the display) extends out of the bounds of the monitors. I can make another issue if you're interested in fixing that.

Who places monitors using turtles anyway ? Feel free to close this issue if you think your work is done here.
Thanks for investigating the issue and explaining it clearly anyway.

commented

One of them is that game doesn't crash, but the black part of the monitor (the display) extends out of the bounds of the monitors.

I'm fairly sure this is caused by the same underlying bug. I imagine there's some problems when a monitor gets (incorrectly) replaced during a resize operation.

Delaying the update to next tick could solve the issue, but as you said it would be ugly and pretty bad to debug if there are other issues with it.

Agreed, but any other solution I can think of ends up being more fragile. For instance, we need to consider the case of two turtles placing monitors next to each other in the same tick (and so both monitors hit the issue with being repeatedly removed/replaced).

We can limit the scope of this by only deferring the block state update, not the whole monitor resize. If we also do this check on chunk load, then things should be relatively foolproof.