Uploaded image for project: 'Minecraft: Java Edition'
  1. Minecraft: Java Edition
  2. MC-162080

PalettedContainer is not threadsafe

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Awaiting Response
    • None
    • 1.14.4, 19w39a, 1.16.2, 21w43a, 1.18 Pre-release 4, 1.18 Pre-release 5, 1.18 Pre-release 7
    • None
    • Community Consensus
    • Lighting
    • Important
    • Platform

      Albeit being used concurrently by multiple threads, PalettedContainer isn't threadsafe. This can cause reading threads to see non-meaningful values, in particular Chunk.getBlockState(...) can return block states that never existed at the respective block pos (in the current session of the game).
      As an example, this can cause issues with the new lighting engine which runs concurrently to the rest of the game logic. As multithreading issues are hardly reliably reproducible, the following steps use breakpoints to pause threads in order to simulate some bad luck. However, I was also able to reproduce the bugs even without breakpoints, just not reliably.

      • Create a new redstone-ready world
      • Set a breakpoint somewhere in the lighting code to prevent the next step from finishing/starting the light propagation. You should configure the breakpoint to only pause the thread triggering it, instead of pausing all threads. For example, you can set a breakpoint at the first line of
        net.minecraft.world.chunk.light.ChunkBlockLightProvider.java
        protected int getPropagatedLevel(long long_1, long long_2, int int_1) {
              if (long_2 == Long.MAX_VALUE) {
                 return 15;
              } else if (long_1 == Long.MAX_VALUE) {
                 return int_1 + 15 - this.getLightSourceLuminance(long_2);
              } else if (int_1 >= 15) {
                 return int_1;
              } else {
                 int int_2 = Integer.signum(BlockPos.unpackLongX(long_2) - BlockPos.unpackLongX(long_1));
                 int int_3 = Integer.signum(BlockPos.unpackLongY(long_2) - BlockPos.unpackLongY(long_1));
                 int int_4 = Integer.signum(BlockPos.unpackLongZ(long_2) - BlockPos.unpackLongZ(long_1));
                 Direction direction_1 = Direction.fromVector(int_2, int_3, int_4);
                 if (direction_1 == null) {
                    return 15;
                 } else {
                    AtomicInteger atomicInteger_1 = new AtomicInteger();
                    BlockState blockState_1 = this.getStateForLighting(long_2, atomicInteger_1);
                    if (atomicInteger_1.get() >= 15) {
                       return 15;
                    } else {
                       BlockState blockState_2 = this.getStateForLighting(long_1, (AtomicInteger)null);
                       VoxelShape voxelShape_1 = this.getOpaqueShape(blockState_2, long_1, direction_1);
                       VoxelShape voxelShape_2 = this.getOpaqueShape(blockState_1, long_2, direction_1.getOpposite());
                       return VoxelShapes.unionCoversFullCube(voxelShape_1, voxelShape_2) ? 15 : int_1 + Math.max(1, atomicInteger_1.get());
                    }
                 }
              }
           }
        
      • /setblock 7 60 7 minecraft:sea_lantern
      • Some worker thread (and the rendering thread) should now hit the breakpoint. You can now disable the breakpoint and resume the rendering thread, but not the light worker thread.
      • Set a breakpoint at the last instruction of the following method, again configuring it to only pause the triggering thread
        net.minecraft.world.chunk.PalettedContainer.java
        private void setPaletteSize(int int_1) {
              if (int_1 != this.paletteSize) {
                 this.paletteSize = int_1;
                 if (this.paletteSize <= 4) {
                    this.paletteSize = 4;
                    this.palette = new ArrayPalette(this.idList, this.paletteSize, this, this.elementDeserializer);
                 } else if (this.paletteSize < 9) {
                    this.palette = new BiMapPalette(this.idList, this.paletteSize, this, this.elementDeserializer, this.elementSerializer);
                 } else {
                    this.palette = this.fallbackPalette;
                    this.paletteSize = MathHelper.log2DeBrujin(this.idList.size());
                 }
        
                 this.palette.getIndex(this.field_12935);
                 this.data = new PackedIntegerArray(this.paletteSize, 4096);
              }
           }
        
      • Place 14 different blocks (other than stone, sandstone and sea lanterns) in the 0 3 0 subchunk (where the sea lantern was placed)
      • This should trigger the second breakpoint from the server thread (and also the rendering thread). Now resume the lighting worker thread. Afterwards, you can disable the second breakpoint and resume all threads.
      • Restart the world
      • Observe that the sea lantern does not emit light

       

      Similarly, you can make blocks transparent

      • Create a new redstone-ready world
      • /fill 6 63 6 8 66 8 minecraft:stone hollow
      • Procede as above, but instead set the sea lantern at /setblock 7 65 7 minecraft:sea_lantern and set the first breakpoint at the first line of
        net.minecraft.world.chunk.light.ChunkBlockLightProvider.java
        protected void propagateLevel(long long_1, int int_1, boolean boolean_1) {
              long long_2 = ChunkSectionPos.fromGlobalPos(long_1);
              Direction[] var7 = DIRECTIONS;
              int var8 = var7.length;
        
              for(int var9 = 0; var9 < var8; ++var9) {
                 Direction direction_1 = var7[var9];
                 long long_3 = BlockPos.offset(long_1, direction_1);
                 long long_4 = ChunkSectionPos.fromGlobalPos(long_3);
                 if (long_2 == long_4 || ((BlockLightStorage)this.lightStorage).hasLight(long_4)) {
                    this.propagateLevel(long_1, long_3, int_1, boolean_1);
                 }
              }
        
           }
        
      • Observe that the light shines through the stone blocks

       

      The two main issues with PalettedContainer are that

      • the underlying data storage is not atomic since block ids can be spread over multiple integer entries, hence non-meaningful block ids can be returned
      • resizing the block state palette is not atomic. During resizing, the data storage and palette field get replaced by new instances and afterwards the data is copied over (and the block ids are even shuffled). This opens the possibility for a reading thread to observe not yet copied entries or an inconsistent storage-palette pair, meaning that the block id is looked up using the wrong palette.

      In the examples above, the second problem caused the lighting thread to observe only air blocks since we timed the threads so that the palette was being resized while the lighting thread was reading. Hence the sea lantern didn't emit light (as it was seen as an air block) and the stone blocks didn't block the light.

       

      While this might not be the most game breaking bug, the fact that it's hardly reproducible makes it nevertheless quite annoying, in my opinion. That is because you won't really be able to track some randomly appearing issue down to this bug for the lack of reproducibility.
      I have assembled a more detailed description and an implementation for an efficient lock-free solution at https://github.com/OverengineeredCodingDuo/mcoptimizations/tree/blockstatecontainer (see BlockStateContainer.java.patch as a starting point). The patches were written for 1.13.2, but the relevant code hasn't changed since.

      In my opinion, the solution is quite short and simple and definitely worth the effort, given that it can save some headaches in the future, especially in case you plan to multithread other parts of the engine.

      Please don't hesitate to ask if you have any questions

      Best,
      PhiPro

        1. image-2019-09-28-13-36-43-436.png
          313 kB
          Philipp Provenzano
        2. image-2019-09-28-13-45-40-779.png
          221 kB
          Philipp Provenzano

            Unassigned Unassigned
            PhiPro Philipp Provenzano
            Votes:
            13 Vote for this issue
            Watchers:
            13 Start watching this issue

              Created:
              Updated:
              Resolved:
              CHK: