Schedule Repeating Task EXIT

I've been looking for a while now to find a graceful way to exit an AsyncScheduledRepeatingTask...

Does anyone know how to exit the thread smoothly?

here is my test code:

Code:

plugin.xteam.getServer().getScheduler().scheduleAsyncRepeatingTask(plugin.xteam, new Runnable()
{
    public void run()
    {
        System.out.println(p.getLocation());
        count++;
        System.out.println(count);
        if (count == 10)
        {
            //code to exit and/or cancel the thread
        }
    }
}, 20L, 5L);

 

Code:java

final int taskID = plugin.xteam.getServer().getScheduler().scheduleAsyncRepeatingTask(plugin.xteam, new Runnable() {
public void run() {
System.out.println(p.getLocation());
count++;
System.out.println(count);
if (count == 10) {
Bukkit.getScheduler().cancelTask(taskID);
}
}
}, 20L, 5L);

 

Wow... I literally found this answer a minute ago in another obscure thread, added it, and came back here to delete the thread.

Thanks though, that's almost exactly what I used.

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

 

I may hope that var 'p' is not an instance of the bukkit api, or else you have an thread bug here

 

Don't call bukkit methods from a separate thread (with the exception of world.getTypeIdAt and the scheduler methods)!
See here for reference.

 

p is defined before the scheduled thread as a 'final SpoutPlayer' object... is this not thread-safe?

I'm still a little fuzzy on the details of why being thread-safe is an actual concern. What exactly is prohibited here..?
The reference is helpful to a point but still confusing because I am not very familiar with threads in the first place.

I was only using the out.println() method as a debugging tool...

Could you explain what you mean by the 'bug' I might have and how i could fix it? And how would p be an instance of the Bukkit API?

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

 

SpoutPlayer extends CraftPlayer which implements org.bukkit.entity.Player, so it is part of the bukkit API.

I would like to point out one thing first: If you never worked with Threads before / don't have sufficient knowledge about concurrency and the (dis)advantages of threading, don't use it. I don't know what you are planning to do with your task, but currently there is no reason not to use a sync task over an async one.
You don't need to worry about all these issues when you just stay in the main thread (= using a sync task).

How multiple Threads interfere with each other when they use the same objects is explained here:
http://docs.oracle.com/javase/tutorial/essential/concurrency/interfere.html
(You might want to consider reading the rest of the concurrency tutorial if want to go deeper into the subject)

A "thread-safe" method means that the method can be safely accessed by multiple threads without any possible problems. Technically, that would look something like:
- ThreadA calls method ... (method is executed ...)
- ThreadB tries to call method before ThreadA is finished --> ThreadB has to wait until ThreadA finished his stuff in the method, only then it can continue

The minecraft server as well as bukkit isn't designed like that. It has one main thread that manages the ticks (20 per second), and only at those ticks stuff is executed. Of course there is some threading involved in the networking with the clients (sending/retrieving data), but that is separated from the rest.
That's why "the rest" doesn't need to be thread-safe, and that's why another thread can not access "the rest" safely.
Doing so will eventually cause some interference (the main thread does something, your custom thread does something else in between, the main thread gets confused and may mess stuff up). Since the server is a pretty complex thing, it can cause everything - from nothing to a harmless exception to a whole corrupted world.

 

I'm wondering how else I would achieve what I would like to do without creating a separate thread. What I have it doing is checking to see if a player moves after doing a teleport call. On my personal server, this prevents users from trying to teleport with enemies nearby. The repeating task continues checking whether or not they have moved since they called the function, if so, cancel the task. Basically I wanted a synchronized delay that constantly checks where the user is located - that way if they try to teleport home, and a monster is nearby, and they stand still for ten seconds, then they get sent home.

 

Well, all you have to do for that is using PlayerMoveEvent, or a syncRepeatingTask firing very often (when you need it). Even when going with the task, you don't need a separate thread.
The great thing about the scheduler is that it provides that repeating task functionality - allowing you to do something repeatedly but without using a separate thread.

The idea is:
1 (using PlayerMoveEvent):
- player performs command, is flagged for being tracked, timer is launched (sync delayed task)
- move event listens when the player moves and cancels the timer when he does
- timer (if it wasn't canceled) performs the teleporting

... or
2 (using sync repeating task):
- player performs command, repeating task is launched which includes a counter and is fired about every 2 ticks
- the task knows the player's initial location and checks at every run if it is still the same
- the task counts up a counter every time it is ran, and when it hits a certain value, it performs the teleport and cancels itself

Pros of the second approach:
+ Processing is only needed in the short "warm-up" time, there's no footprint when no one does a teleport right now.
+ Processing is only needed for the one specific player - you don't have to track movement of all players and process that.

I can't really think of any downsides of the second approach, it's not even more complicated in this case.
Just make sure your task is sync and not async - for the reasons previously discussed.

 

I was thinking of how to use the onPlayerMove but I've always been cautious because it could possibly create a lot of overhead.

This is the exact code I have for this section:

Code:

final SpoutPlayer p = (SpoutPlayer) player;
counts.put(p.getName(), 0);
final Location save = p.getLocation();
player.sendMessage(ChatColor.RED + "You cannot teleport with enemies nearby");
player.sendMessage(ChatColor.RED + "You must wait 10 seconds");
taskIDs.put(player.getName(), TeamPlayerListener.xteam.getServer().getScheduler().scheduleSyncRepeatingTask(TeamPlayerListener.xteam, new Runnable()
{
    public void run()
    {
        if (damagedByPlayer.contains(player.getName()))
        {
            player.sendMessage(ChatColor.RED + "You were attacked!");
            damagedByPlayer.remove(player.getName());
            counts.remove(p.getName());
            Bukkit.getScheduler().cancelTask(taskIDs.remove(player.getName()));
        }
        if (Math.round(p.getLocation().getX()) != Math.round(save.getX()) || Math.round(p.getLocation().getY()) != Math.round(save.getY()) || Math.round(p.getLocation().getZ()) != Math.round(save.getZ()))
        {
            player.sendMessage(ChatColor.RED + "You moved!");
            counts.remove(p.getName());
            Bukkit.getScheduler().cancelTask(taskIDs.remove(player.getName()));
        }
        if (taskIDs.containsKey(player.getName()))
        {
            int temp = counts.remove(p.getName());
            if (temp == 40)
            {
                teleHQ(player);
                Bukkit.getScheduler().cancelTask(taskIDs.remove(player.getName()));
            }
            temp++;
            counts.put(p.getName(), temp);
        }
    }
}, 0L, 5L));

I would put the whole function but it is a bit long..
The only thing I changed was the Async to Sync now.
1) Is this thread-safe?
2) If it is or is not, how could I improve it?

 

1) Thread-safety is not needed anymore, because you are staying in the main thread. But yes, what you are doing is safe to use.

2) Only improvement I would take note of would be: Only call p.getLocation() once in your method and store it in a local variable - don't repeatedly call it. The reason is that getLocation() always creates a new Location instance - so calling it multiple time causes unneeded heap allocations.
Umm, and I think conversion from "exact coordinates" to "block coordinates" doesn't use Math.round but Math.floor. No big difference I think. You could call someLocation.getBlockX() to automatically get an int to compare (I think that's what you want, to tolerate little movement in the current block). That method uses Math.floor as well, so not really a performance improvement.

 

Yes I realize calling p.getLocation() uses unnecessary memory, it was more of an on the fly, "get the code to work correctly" sort of move on my part. Thanks for the tip on the getBlockX(), I never bothered to scan through all of the methods - might have helped.

Anyway, thanks.