Uploaded image for project: 'Spigot'
  1. Spigot
  2. SPIGOT-1097

Unsafe Async implementation of ConcurrentMap in ChunkRegionLoader

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Invalid
    • Icon: Minor Minor
    • None
    • None
    • None

      (This problem occures very rarely or maybe never if no plugin calls sync saveChunk() via the scheduler. The Problem in itself is 'Critical'. The rarity might make it 'Minor')

      Some Versions ago you changed the Code from the ChunkRegionLoader from:

          private java.util.LinkedHashMap<ChunkCoordIntPair, PendingChunkToSave> pendingSaves = new java.util.LinkedHashMap<ChunkCoordIntPair, PendingChunkToSave>();
      

      to the more default-Implementation of Minecraft:

          private Map<ChunkCoordIntPair, NBTTagCompound> b = new ConcurrentHashMap();
      

      While this is generally a good choice I want to point out a design-Flaw in the Class.

      Method c() uses a Iterator to walk over the Entries to be saved. Iterators are not Thread-Safe even if using a ConcurrentHashMap. The code has to be syncronized or written differently. An argument could be "we dont call this method async anyway" but then again the use of ConcurrentHashMap in the first place would be questionable.

          public boolean c() {
      [...]
      ChunkCoordIntPair chunkcoordintpair = (ChunkCoordIntPair) this.b.keySet().iterator().next();
      [...]
      

      When can this problem occur?
      It sould be possible to Crash the Server by using 100% valid sync-only code by calling .saveChunk()
      This can happen because Minecraft itself calls this Methods from an async thread via
      net.minecraft.server.FileIOThread.c(SourceFile:37).

      Reproduction
      This is of course hard to test as it is not easy to force a CME/NPE of an infrequently called Method. But using an Iterator over a ConcurrentHashMap in an not syncronized Method should be enough to know this will result in problems at some point. At least from a programmers perspective this should be sufficient.

      I personally ran into this problem about 3-4 Times on a very populated Server over 3-4 Month - but i am using a heavily modified CB/Spigot Build with various optimizations. Specific to this problem i am using a chunkSavingQueue that distributes the regular worldsaving over a longer period of time to prevent lags and save all worldchunks more frequently. This 100% sync code change was able to result in the problem as it heavily increased the chance to bump into the async net.minecraft.server.FileIOThread.c() code.

            Unassigned Unassigned
            Kademlia Kademlia
            Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

              Created:
              Updated:
              Resolved: