Please read this personal appeal

That was in one way directed at you in particular

 

I really don't understand why plugin developers are using Aysnc or parallel threads. I can only think of a few real use cases. Writing files to a disk, sending data to a SQL database, ChunkSnapshot analysis (aka Dynmap, Logging plugins).

If you think your plugin is running slow - multithreading will not make it run faster. 98% plugins are MUCH better off optimizing themselves for single-threaded code than ever touching a 2nd thread.

I'd be happy to review multithreaded code ( @bergerkiller seems to be doing a good job so far though) as well, but the quick rule of thumb is that if you don't know if it's safe, it's not.

 

Games with complex AI requirements

 

Now that this topic is here I'd like to ask a quick question:

Say you want to do something every x amount of time (for simplicity let's say toggling the rain), and this will go on the whole time the plugin is enabled.

First, am I right in understanding this should be sync, not async, and second, what performance cost would this have on the server?

E.g. would every one minute be fine but every five seconds be damaging? Or is even every minute too often?

 

That depends entirely on how complex your task is. A really simple task which just checks a variable, and sends a message, for example, would be fine to run more than once a second.

But in general, repeating tasks need to be kept as short as possible.

As regards sync/async - it depends on if you're making unsafe Bukkit API calls. The rest of this thread discusses that in detail Async tasks are needed if your plugin needs to do some complex calculations which don't directly affect the world - e.g. in my ChessCraft plugin, the AI engine runs in an async task, and reports its move back to the main server thread. Then I have a scheduled sync task which runs once a second to update chess clocks and check if there are any completed AI moves to process. The sync task does the minimum needed - updates clock counters, repaints signs, and checks for a boolean aiHasMoved flag (and updates some blocks to reflect the AI board position if necessary).

 

So async is like the BackgroundWorker from c#? e.g. if you need to read/write large amounts of data from disk or do a lot of work just to get a result you use it. But any changes to the world (spawning mobs, changing blocks, etc.) should be done with sync?

 

Yep, pretty much.

 

@Afforess is pretty much right: you never need an async task or thread. The only purpose (or purpose I see in them) is:
- Perform tasks that have to be done in second/msecond interval and not ticks
- File operations, like how SQL databases write/read their data to/from file in a separate thread.
- Long operations on certain objects; for example; I use an async task to fix the lighting of a copy of a chunk
- Constant operations: if you want a long list of (ever increasing) operations to be performed as fast as possible without having tick lag/slower processing.

Things they should NEVER be used for:
- (repeating) delays that can rely on ticks (we have a scheduler for that)
- Operations that modify/loop through collections managed on the main thread (entities, chunks somewhat, player list)
- Operations that cause events which have to be processed on the main thread (for real)
- Operations that have a very little yet existent chance for the above occuring

A lot of Bukkit functions have been synchronized already (like the chunk map I believe), but even then, getting a chunk that wasn't loaded already will cause a chunk load event on your thread. This will/can crash a lot of other plugins.

I decided to take it personally: I'm going to add a simple exception on low-managed listeners which check the main thread.

Code:

    private static final Thread currentThread = Thread.currentThread();
    public static void checkThread() {
        final Thread t = Thread.currentThread();
        if (t != currentThread) {
            IllegalAccessError err = new IllegalAccessError("Synchronized code got accessed from another thread: " + t.getClass().getName());
            StackTraceElement[] stack = new StackTraceElement[t.getStackTrace().length - 2];
            for (int i = 0; i < stack.length; i++) {
                stack[i] = t.getStackTrace()[stack.length - i + 1];
            }
            err.setStackTrace(stack);
            throw err;
        }
    }

This will ensure that no damn plugin can ruin it anymore.

Result if I mess things up on purpose:

Code:

17:51:32 [INFO] java.lang.IllegalAccessError: Synchronized code got accessed from another thread: com.bergerkiller.bukkit.nolagg.ChunkScheduler
    at com.bergerkiller.bukkit.nolagg.ChunkScheduler.run(ChunkScheduler.java:143)
    at com.bergerkiller.bukkit.nolagg.ChunkOperation.execute(ChunkOperation.java:60)
    at org.bukkit.craftbukkit.CraftWorld.spawnCreature(CraftWorld.java:323)
    at org.bukkit.craftbukkit.CraftWorld.spawn(CraftWorld.java:655)
    at org.bukkit.craftbukkit.CraftWorld.spawn(CraftWorld.java:818)
    at net.minecraft.server.World.addEntity(World.java:883)
    at org.bukkit.craftbukkit.event.CraftEventFactory.callCreatureSpawnEvent(CraftEventFactory.java:259)
    at org.bukkit.plugin.SimplePluginManager.callEvent(SimplePluginManager.java:339)
    at org.bukkit.plugin.RegisteredListener.callEvent(RegisteredListener.java:58)
    at org.bukkit.plugin.java.JavaPluginLoader$72.execute(JavaPluginLoader.java:767)
    at com.bergerkiller.bukkit.nolagg.NLEntityListener.onCreatureSpawn(NLEntityListener.java:33)

 

@bergerkiller - nice idea, shaming plugin developers who abuse the API may be the best route to go.

 

@Afforess just what I thought, and the idea behind it is pretty simple. lowest-level Hooked into all possible cross-thread events (events that can occur from fired methods, not all to keep it simple). Added all of it in NoLagg, already spotted XAuth firing player.teleport in a separate thread.

 

LOL, shaming other plugin developers.
/win

Question:
Is this code threadsafe? Is it worth threading?
https://github.com/tips48/RushMe/blob/master/src/main/java/com/tips48/rushMe/util/RMLogging.java

EDIT by Moderator: merged posts, please use the edit button instead of double posting.

 

Looks safe...but I would need usage context to be 100% sure.

 

Aand now it seems that ButtonWarp teleport players from another thread and that MCBans kicks players from another thread.

 

MCBans is badly programmed anyway, it lags the server whenever a player joins (since it needs to fetch some data from an external server).

 

Now added BLOCK_PHYSICS in the check too. I wonder how many plugins deal with block editing in another thread...I expect a wave of plugins that do this for some reason...

 

I just got an idea for some kind of addon/library for plugin devs, or if done cleanly even an extension to the bukkit API itself. In the latter case, there would be a couple of methods introduced in CraftServer, CraftPlayer etc., that are thread safe, like for example Player.teleportAsync(Location)
Internally it would look something like (warning, not actual/tested java code)

Code:

public void teleportAsync(Location loc)
{
  getServer().getScheduler().scheduleSyncTask(new Runnable(this, loc) {
    Player p; Location l;
    Runnable(Player p, Location l)
    {
      this.p = p; this.l = l;
    }
    void Run()
    {
      p.teleport(l);
    }
  }
}

Cause it's really as simple as that to do some calls to the Minecraft server from your async thread/task.
As it is always the same, having it in Bukkit itself would be best to prevent code duplication among plugins.
Then just put a big warning in the javadoc for all the non-asnyc methods that they must not be called from any other threads, refer to the *Async method for it, and plugin devs should have no more excuse for doing bad stuff.

Of course this only works for methods that do not return anything meaningful, or where the return value can be neglected mostly. But it would be a start.

 

@Borch Sadly this is not what will ever be added. If it can be done from a plugin, it's not added. It's to limit the size of the project I guess. However, I do believe they should synchronize the events, or at least throw an error if an event is called from another thread. (like I do in NoLagg)
It's basically adding 10 lines of code in the plugin manager / callEvent...

 

MCBans and MCBouncer are probably the two most improperly coded plugins I've seen in regards to threading. And I don't really bat an eye when someone says their server is crashing when either of these are installed. MCBans is fortunately getting a re-write (from what I was told) - to get rid of all the pointless threading.

 

@Sleaker good to read that...never assume popular plugins are good plugins I suppose. Also, the author of ButtonWarp is already fixing that issue, great thanks to him ofc

 

if anyone is wondering why MCBouncer is really bad check this: https://github.com/pdaian/MCBouncer.../java/com/mcbouncer/plugin/MCBouncer.java#L97

MCBans is not nearly as bad as this.

 

Well, that makes sense I guess, although it would really help to break the rules here... But well, then maybe something like a must-have lib that offers these wrapped functions should be created.
About synchronizing the events, that would probably not be the right spot, as not just the bukkit event needs to be synchronized but also the code that does the actual work inside the Minecraft server. So in case of a teleport, not the async teleport event that gets triggered later is the problem, but the async calling of the Player.teleport method itself is already unsafe. But I do agree that there should at least be some sanity checks whether certain methods are called from another thread.

 

@Borch well you can do the following:

Code:

public static Object mainlock = new Object();

//somewhere
synchronized (mainlock) {
    //main Bukkit game loop here
}

And in all methods that need to be synchronized, add a synchronized (mainlock) field around the method. For example:

Code:

public void teleport(Location to) {
    synchronized (Bukkit.mainlock) {
        //perform teleportation here
    }
}

This way async tasks will automatically halt whenever they try to access synchronized methods - it's locked. But I prefer having an IllegalAccessException better, since you then get a stacktrace instead of a thread lock.

Of course, also possible to perform all coding in the main loop in a synchronized block to allow teleport events to occur after the main loop finished (or right before it)

Code:

//main Bukkit game loop
synchronized (mainlock) {
    //perform loop operations
}

 

This is ridiculous, I get around 2 new plugins that break the rules every day?! And I was wondering why people were complaining of concurrent modification issues...

Most common mistakes are:
- getChunk
- player.teleport
- block.physics
Worse is that the situation worsens once one plugin breaks the rules. For example:
1. Plugin teleports player to another world from another thread
2. Other plugin, which uses the teleport event, calls getChunk
3. NoLagg reports the load call on another thread
4. NoLagg reports the player teleport call shortly after

Seriously, PLEASE (big please with some sugar on top) stop calling Bukkit methods from another thread? Please? You are kinda destroying the internal Bukkit mechanics with your threads...

 

All of my rage. All of it.

 

Unfortunately I had to give up this fight. The amount of plugins that do this is just tremendous, it's like throwing a hook into a pool of swarming fish. I have lost all hope in plugin developers today, even respected plugins break the rules and mess everything else up. I'll just say 'throw x plugin away it's buggy as hell', nothing else I can do.

List of malbehaving plugins:

And I bet a lot of other plugins do this already. Sadly a lot of these plugins are already outdated and have no updated versions, so nothing I can do about it. If this goes on like this, Bukkit will be dead within a year

 

@bergerkiller

If you were, *cough*, designing a new API, do you think you could fix the thread safety problems in it?

My idea is to wrap the api like this:

Code:

public class CraftPlayer implements Player {
...
public void teleport(Location to) {
    if (Thread.currentThread() == MainThread) {
          //do task
    }
    else {
          //add runnable task to queue, processed next tick
    }
}
...
}

 

@Afforess It's a little tough to start a new Bukkit server project 'like that' while maintaining compatibility with existing plugins. And most plugins will assume that the player location changed after that call and will thereby fail if it would suddenly queue it.

It's a lot better to synchronize everything using a single main lock object. This way async-accessing tasks/threads will wait out the main thread before executing the code, while main-thread accessed code will run as normal.

The problem is not really Bukkit, it's the vast amount of plugins that assume multi-threading 'just works'...

 

Is the same true for getServer().broadcastMessage?

 

@sorklin no, but if Bukkit decides to add an event for it, then it is.

 

Now this really surprised me ... I was totally aware that knowledge about threading/concurrency is not very common, but I assumed that at least the bigger plugin devs would either have a clue of it or just keep their hands off it.
That a lot of crappy beginner developers hearing that threading increases performance fall for the fault to use it without having any idea of it also seems apparent, but their plugins usually don't become popular.

It is shocking to see how threading is being abused, and I honestly don't think that is up to bukkit to do a work around. The whole server is not designed to be accessed by a funky custom thread, trying to do so would require recoding a lot of native code.
Only synchronizing a bit of stuff of bukkit isn't enough - it might then be able to call events thead-safely, but actual changes to the world happen in nms code, which is partly threaded itself (all the network threads managing clients for example), but the rest is not.
Locking "everything" would pretty soon cause a dead-lock or at least severe performance issues somewhere, I'm pretty sure of that.

In my opinion, a wise and not really complicated (but apparently necessary) way would be like you did: forbid access and throw errors at every method that is called from a different thread and that is not thread-safe.
If that is officially implemented in bukkit - together with an official explaining post why suddenly errors are everywhere - it has the potential to effectively separate the wheat from the chaff.