Need Minecraft Source code for Concurrent Modifications

Discussion in 'Plugin Development' started by adamjon858, Jun 20, 2012.

Thread Status:
Not open for further replies.
  1. Offline

    adamjon858

    I'm not sure what is causing this but a plugin I wrote is causing this error every few hours:

    Code:
    2012-06-19 21:43:17 [SEVERE] java.util.ConcurrentModificationException
    2012-06-19 21:43:17 [SEVERE]    at java.util.AbstractList$Itr.checkForComodification(AbstractList.java:372)
    2012-06-19 21:43:17 [SEVERE]    at java.util.AbstractList$Itr.next(AbstractList.java:343)
    2012-06-19 21:43:17 [SEVERE]    at net.minecraft.server.World.tickEntities(World.java:1174)
    2012-06-19 21:43:17 [SEVERE]    at net.minecraft.server.MinecraftServer.w(MinecraftServer.java:558)
    2012-06-19 21:43:17 [SEVERE]    at net.minecraft.server.MinecraftServer.run(MinecraftServer.java:459)
    2012-06-19 21:43:17 [SEVERE]    at net.minecraft.server.ThreadServerApplication.run(SourceFile:492)
    2012-06-19 21:43:17 [SEVERE] Unexpected exception
    java.util.ConcurrentModificationException
            at java.util.AbstractList$Itr.checkForComodification(AbstractList.java:372)
            at java.util.AbstractList$Itr.next(AbstractList.java:343)
            at net.minecraft.server.World.tickEntities(World.java:1174)
            at net.minecraft.server.MinecraftServer.w(MinecraftServer.java:558)
            at net.minecraft.server.MinecraftServer.run(MinecraftServer.java:459)
            at net.minecraft.server.ThreadServerApplication.run(SourceFile:492)
    Now I know that this means I must be modifying a list in my plugin at the same time that the Minecraft server is trying to iterate over it (or vice versa). However, I'm not sure what it could be?

    Does anyone have a way of knowing what is being looped over at World.java:1174? I would guess entities but I'm 99% sure my code doesn't loop over entities (although it does, modify them but not within a loop).
     
  2. Offline

    p000ison

    So I hope I dont get banned because I post this:
    Code:
        Iterator iterator = this.tileEntityList.iterator();
     
        while (iterator.hasNext()) {
          TileEntity tileentity = (TileEntity)iterator.next();
     
          if ((!tileentity.l()) && (tileentity.world != null) && (isLoaded(tileentity.x, tileentity.y, tileentity.z))) {
            tileentity.q_();
          }
     
          if (tileentity.l()) {
            iterator.remove();
            if (isChunkLoaded(tileentity.x >> 4, tileentity.z >> 4)) {
              Chunk chunk = getChunkAt(tileentity.x >> 4, tileentity.z >> 4);
     
              if (chunk != null) {
                chunk.f(tileentity.x & 0xF, tileentity.y, tileentity.z & 0xF);
              }
            }
          }
        }
    line 1174 is TileEntity tileentity = (TileEntity)iterator.next();

    So if its a problem to post this, please simply remove this post. Thanks :p
     
  3. Offline

    Korvacs

    Use the Synchronised keyword on the list your trying to modify.
     
  4. Offline

    adamjon858

    I've checked and I'm not modifying or looping over any Minecraft lists. I have some internal lists that I loop over that contain references to Entity Id's but those are currently ConcurrentHashMaps.

    I've changed those to Collections.synchronizedMap to see if it fixes it...we shall see.


    That's odd. My plugin is a custom mob spawning plugin:

    https://github.com/adamjon858/Mobster/tree/master/src/com/blockempires/mobster

    But it doesn't use any tile entities or the default spawners at all. I do get the block of a location inside of threads but as far as I know the minecraft getBlockX,Y,Z functions are all threadsafe. I'm not modifying blocks at all.

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: May 26, 2016
  5. Offline

    Njol

    Don't use separate threads (including asynchonous tasks) if you don't know what you're doing.
     
    zhuowei likes this.
  6. Offline

    adamjon858

    I've used threads a lot in Python and C++.

    Just not a ton with Java. My plugin was working fine for a long time but now that I get 150+ players online with it, it has occasionally started crashing because of thread issues. Just trying to diagnose them.

    And there's no way to setup automated spawning processes without using threads.
     
  7. Offline

    desht

    Sure there is. Use a sync task - it runs in the main server thread' so just take care not to spend too long in it.

    Using your own thread to call NMS methods (even via Bukkit) is not an option. It might appear to work fine when you test it on an empty dev server but as you've seen, it isn't reliable, and getting a concurrent mod exception is the least of your worries; world corruption is also a possibility.
     
  8. Offline

    Korvacs

    Code:
    Synchronised (object) {
     
    }
    Solves almost every threading issue your likely to encounter.
     
  9. Holy shit, no, just no. Simply no! It won't help you AT ALL in the context of a minecraft server, and even less when you don't know what it actually does (like you appear to).

    Seriously, don't point out blantly wrong facts if you just don't know what you are talking about, please.
    http://download.oracle.com/javase/tutorial/essential/concurrency/
    ^ This explains some of the basics of synchronization in Java.

    Anyway, as desht pointed out, the BukkitScheduler with its sync tasks works out almost every time. You hardly ever need to compute slow enough operations that don't have to access the world/server.
     
    zhuowei likes this.
  10. Offline

    Korvacs

    Ok, clearly you have no idea what the Synchronised keyword does in that context...
     
  11. I really hope you are not being serious. I'll explain it anyway what it actually does (even though the article I linked does that as well):

    The synchronized keyword in Java maked the current thread aquire the so-called "intrinsic lock" of the object it is provided (when you use it as a modifier for a method, said object is simply the instance of class the method is being called on). At the end of the block (or method), the lock is released again.

    It aquires that lock, and as long as the thread has that lock, no other thread can aquire it.
    In reverse, that means, if your thread reaches the synchronized keyword and said lock is already aquired by another thread, your thread blocks (waits) until the lock is free again.

    The thing is, owning the lock has no point at all when another thread accesses the object (or whatever you try to protect with your lock) directly without first synchronizing on the exact same lock.

    Well, the minecraft server doesn't do that for its unthreaded parts like the tick system, so if you go ahead and get the intrinsic lock of anything, it does not create any sort of thread safety:

    The main server thread can continue happily accessing the object while your thread is in the synchronized block doing nasty stuff. Thread safety is only provided if ALL accesses to the thing you want to make safe aquire the intrinsic lock first.

    Writing "synchronized" in your code will NOT magically make all the stuff in it execute in a single operation without any other thread having a chance to do its thing. No buddy, that's not how threading works.
     
  12. Offline

    Korvacs

    lol, ok you clearly dont understand then, if you lock using synchronised then ALL threads block, regardless of them using the synchronised keyword themselves.

    Feel free to go try this for yourself, since you have apparently never tried it.
     
  13. I agree to Bone008 as it's handled about the same in c++. Why should ALL threads block, if you lock only ONE variable? That doesn't make any sense at all. If you want to block all threads, then why would you need to lock one specific variable? I don't see the logic behind this.
     
  14. Offline

    Korvacs

    You might not see the logic but thats how it works, the lock provides the current thread with exclusive access to that variable while you have the lock and blocks all other threads thats all it does, it doesnt do as you believe which is make it a requirement that all threads must use a lock to access that variable, because you are not locking the object exclusively for the entire time. The idea is to provide you with a method to create your own concurrent models or so that if you have an object which is written to 5% of the time but iterated through the rest of the time, you can lock the object for that 5% of the time and block the other threads.

    To me its completely logical to lock an object for the (potentially) extremely small period of time that you need to write to an object and block the other threads that iterate...but perhaps thats because ive had to do quite alot of this while writing servers in the past...

    You can use it so that all threads accessing it use a lock on the object, but then you might aswell just declare the object as synchronised in the first place.

    Also C++ handles it exactly as i have described, not as you believe...

    If you dont believe me then try it.. Create three threads, one that writes to a list and uses synchronised on the list object while doing so, and two that read from the list and see if the two threads get blocked while doing so.
     
  15. Korvacs I don't know where you are getting your information from, but what you say is kind of ... untrue, to put it nicely.

    Correction: the lock provides the current thread exclusive access to that intrinsic lock as long as you have it and blocks all other threads who are trying to acquire that lock.
    That's where the key problem lies. Another thread that isn't aquiring the intrinsic lock before accessing the variable is not blocked, as you seem to believe. I stated that earlier, and I'll state it again.
    I'll also provide reference: http://docs.oracle.com/javase/tutorial/essential/concurrency/locksync.html
    The last two sentences are the most important part.

    You know, that doesn't even make sense if what you claim would be true.
    Let's assume for a second that the synchronized keyword for some weird reason makes all operations on the inside atomic and magically stops everything else from executing while it's active (which it doesn't, that's what I'm trying to bring across, in case you noticed).
    When the thread that does the iteration does that without having the lock, that means your 5%-of-the-time-writing thread can happily aquire the lock without problems. You actually agree on that, that's nothing special.
    So when that happens and the magically atomic writing operation happens in one rush, the object will still have changed for the iterating thread (that was interrupted while it iterated).
    That means the state is different, and for most java iterators throws a ConcurrentModificationException.

    Like I said, that can happen even when you were right.


    I hope those servers were not for something important. The problem with your way of thinking is explained above.

    I'm sorry to destroy your ideology, but you can't declare attributes/fields as synchronized in Java.
    It wouldn't make any sense as well, if you think about it. The power of the intrinsic lock allows you to keep it owned over multiple accesses to the object - that's viable if you for example iterate. Imagine the lock would be aquired every time the variable is accessed, that kind of defeats its purpose.
     
  16. Offline

    Njol

    Please always post proof for what you're stating (this applies to all of you, not only Korvacs).

    E.g. take a look at this quote from the official Java tutorials (http://docs.oracle.com/javase/tutorial/essential/concurrency/locksync.html):
    Conclusion: As Minecraft does not in any way try to aquire any locks it's thread will never block, no matter how often you write 'synchronized' in your code.

    Edit: Ninja'ed by Bone who posted a reference this time ;)
     
    Bone008 likes this.
  17. Offline

    adamjon858

    Wow. This thread blew up lol.

    I am using a Sync Task not an Async
    Code:
    task = Bukkit.getServer().getScheduler().scheduleSyncRepeatingTask(plugin, this, 100, 20);
    
    I think my problem is that some of the stuff I loop over in the Runnable can also be accessed by Events that fire. However, everything I loop over that can be modified is a ConcurrentHashMap....

    And it also doesn't explain why the error is in the Minecraft.world file and not in my plugin's file.

    I'm only more confused!

    x.x
     
  18. Offline

    Korvacs

    Code:
    private List<String> privateList;
     
    public synchronized List<String> getList() {
            return privateList;
        }
    How to get a synchronised object, tada? I dont have time to write up an example of the locking tonight so expect one tomorrow.
     
  19. Offline

    desht

    Please just stop. You don't know what you're on about.

    OK. Technically not a thread then :)

    Well, something's changed the NMS World's tileEntityList while it's being iterated over. The fun part is finding out what, exactly. Are you certain the exception is caused by your plugin? You're not using separate threads, and your plugin makes no direct reference to any net.minecraft.server code, so it seems unlikely that you're doing anything to cause this...

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: May 26, 2016
  20. Offline

    adamjon858

    That's the thing I modify LivingEntity's a lot but NEVER do I touch TileEntity's
     
  21. Offline

    Korvacs

    Sorry appears i was wrong about this, doesnt even work in C# so i have no idea where i got the idea from!

    However the synchronised list snippet i showed does work, the compiler implements automatic locking on the object in that case.

    Oh well atleast i was man enough to admit my mistakes and wasnt a douche about it.

    adamjon858

    For the purpose of testing you might want to start going through your code and commenting out methods which have the potential to cause this problem until you find out specifically which is causing it.
     
  22. Offline

    Insanehero

    anyone solve this?
     
Thread Status:
Not open for further replies.

Share This Page