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

Async FileSavingProcess will cause chunk overwrites/data loss ( Fix included )

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Duplicate
    • None
    • Minecraft 1.9.4, Minecraft 1.10
    • None
    • Unconfirmed

      Hello, this report fixes a saving-bug that has been in minecraft for years.

      What is the problem? What happens?
      Sometimes Chunks in the world wont be saved or overwritten by a newly generated chunk. The information of these chunks will be lost. This even happens partially without any error messages.

      When does this happen?
      Very rarely. This is mostly a problem of Servers with a high player count and a bigger playable area. At ~100 Players this problem occurs several times a day. With 10 players this problem might never occur.

      What causes this?
      The background process that saves already unloaded chunk-data to the specific regionfiles (.mca). This is happening because the service handling the currently opened regionfiles hands out references to files. This breaks synchronization-protocol.

      Was this reported before?
      Yes, several times without the knowledge of why, where or cause. An example would be MC-10976

      Why a new report?
      This report contains exact information as to why this happens and how to fix this.

      Technical background information
      I am using the spigot-namings. See my report here for additional information: https://hub.spigotmc.org/jira/browse/SPIGOT-2385

      • FileIOThread is the BackgroundService to save already unloaded Chunks to the specific RegionFile. This service is async
      • RegionFileCache holds a bunch of cached RegionFiles for loading/unloading data. FileIOThread regularly inserts data via this. Here is the problem, RegionFileCache hands out references to files!
      • ChunkRegionLoader uses RegionFileCache for different lookups/loading/unloading. For example it checks if a chunk already exists in a region file or not very frequently.

      When does this problem occur?
      Exactly when the RegionFileCache is viewed as "full" and a cleanup is triggered. The cleanup will remove all references to regionfiles from the RegionFileCache and close all regionfiles.

      FileIOThread tries to saves data every X milliseconds to a regionfile.
      If it already started a new try to save data - thus getting a reference to a file from ChunkRegionLoader - and directly after this the RegionFileCache cleanup is started then the data FileIOThread is saving in that moment will be lost. Additionally it is possible the data gets corrupted and minecraft will generate a fresh chunk at that location the next time it is requested.

      How to reproduce?
      As a developer this is very easy. Change the RegionFileCache limit from 256 to 2. This will heavily increase the frequency this problem occurs. This should be enough to spam the console with saving-errors.

      Reprodution of chunk-regenerations
      The best way is to change the limit to 2 as seen above

      • Create a flat world and generate enough chunks in an area
      • Create a normal world
      • Copy all the regionfiles from the flat world to the normal world (dont overwrite the level.dat)
      • Start up the server and fly around in gamemode 3.
      • The console will be full of errors. About every 1-2 minutes there will be a newly generated chunk will appear in the previously flat area.

      How to fix the synchronization/reference problem?
      One way to fix this to no longer give out references to files that could be unloaded at any time. Instead the service managing the references should be the only one to know about them.
      This is possible with relatively low effort.

      As the general implementation of RegionFileCache is faulty the method "c" and "d" need to be rewritten. They are the problem as they hand out references to region files. Instead we can change them to hand out the NBTData directly and mark them syncronized. With this setup the syncronization actually works:

      Here is an example that has been tested. Only ~10 lines of code need to be changed in total.

      Current implementation

          // Kade possibly broken by FileIOThread! too
          @Deprecated
          public static DataInputStream c(File file, int i, int j) {
              RegionFile regionfile = a(file, i, j);
      
              return regionfile.a(i & 31, j & 31);
          }
      
          // Kade is broken by FileIOThread! This will return a reference to a file that may be removed before it is used!
          @Deprecated
          public static DataOutputStream d(File file, int i, int j) {
              RegionFile regionfile = a(file, i, j);
      
              return regionfile.b(i & 31, j & 31);
          }
      

      Fixed implementation

          public static synchronized NBTTagCompound fixedc(File file, int i, int j) throws IOException {
              RegionFile regionfile = a(file, i, j);
      		DataInputStream datainputstream = regionfile.a(i & 31, j & 31);
      		if (datainputstream == null) return null; // ChunkRegionLoader loadChunk
      		return NBTCompressedStreamTools.a(datainputstream);
          }
      
          
          public static synchronized void fixedd(File file, int i, int j, NBTTagCompound nbttagcompound) throws IOException {
      		RegionFile regionfile = a(file, i, j);
      		DataOutputStream dataoutputstream = regionfile.b(i & 31, j & 31);
      		NBTCompressedStreamTools.a(nbttagcompound, (DataOutput) dataoutputstream); // from ChunkRegionLoader b(...)
      		dataoutputstream.close();
      	}
      

      Let me know if additional information is needed.

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

              Created:
              Updated:
              Resolved: