BukkitRunnable not stopping!

Discussion in 'Spigot Plugin Development' started by gluebaby, May 4, 2017.

  1. My BukkitRunnable is not cancelling properly with the .cancel() method. I'll post some code. No errors, the task just repeats.

    Code (Text):
    package me.undeadguppy.sab;

    import java.util.HashMap;

    import org.bukkit.Bukkit;
    import org.bukkit.ChatColor;
    import org.bukkit.entity.Player;

    public class BanManager {

        private static BanManager inst;

        public static BanManager getInstance() {
            if (inst == null) {
                inst = new BanManager();
            }
            return inst;
        }

        public void sendNotification(String msg) {
            for (Player pl : Bukkit.getOnlinePlayers()) {
                if (pl.hasPermission("simpleautobans.notify")) {
                    pl.sendMessage(msg);
                }
            }
        }

        public void stopTask(String name) {
            if (activeBans.containsKey(name)) {
                activeBans.get(name).cancel();
                activeBans.remove(name);
            }
        }

        public HashMap<String, BanTask> activeBans = new HashMap<String, BanTask>();

        public void startAutoBan(String name, String reason) {
            activeBans.put(name, new BanTask(Main.getInstance(), name, reason));
            activeBans.get(name).runTaskTimer(Main.getInstance(), 0L, 20L);
            String warnmessage = ChatColor.translateAlternateColorCodes('&',
                    Main.getInstance().getConfig().getString("Warn-Message").replaceAll("%PLAYER%", name)
                            .replaceAll("%REASON%", activeBans.get(name).getReason())
                            .replaceAll("%DELAY%", String.valueOf(Main.getInstance().getConfig().getInt("Delay"))).trim());
            sendNotification(warnmessage);
        }

    }
     
    Code (Text):
    package me.undeadguppy.sab;

    import org.bukkit.Bukkit;
    import org.bukkit.ChatColor;
    import org.bukkit.scheduler.BukkitRunnable;

    public class BanTask extends BukkitRunnable {

        private Main pl;
        private int delay;
        private String name;
        private String reason;
        private int id;

        public BanTask(Main pl, String name, String reason) {
            this.id = getTaskId();
            this.pl = pl;
            this.name = name;
            this.reason = reason;
            delay = pl.getConfig().getInt("Delay");
        }

        public int getTaskId() {
            return id;
        }

        public int getTimeLeft() {
            return delay;
        }

        public String getName() {
            return name;
        }

        public String getReason() {
            return reason;
        }

        @Override
        public void run() {
            String bancmd = ChatColor.translateAlternateColorCodes('&',
                    pl.getConfig().getString("Ban-Command").replaceAll("%PLAYER%", getName()) + this.getReason().trim());
            if (delay <= 0) {
                Bukkit.dispatchCommand(Bukkit.getServer().getConsoleSender(), bancmd);
                BanManager.getInstance().stopTask(this.name);
                return;
            } else {
                delay--;
            }
        }

    }
     
    Code (Text):
    package me.undeadguppy.sab.autobancmd;

    import org.bukkit.ChatColor;
    import org.bukkit.command.Command;
    import org.bukkit.command.CommandExecutor;
    import org.bukkit.command.CommandSender;

    import me.undeadguppy.sab.BanManager;
    import me.undeadguppy.sab.Main;

    public class AutoBanCommand implements CommandExecutor {

        private Main pl;

        public AutoBanCommand(Main plugin) {
            this.pl = plugin;
            pl.getCommand("autoban").setExecutor(this);
        }

        @Override
        public boolean onCommand(CommandSender sender, Command cmd, String label, String[] args) {
            if (cmd.getName().equalsIgnoreCase("autoban")) {
                if (sender.hasPermission("simpleautoban.command")) {
                    if (args.length == 0) {
                        sender.sendMessage(ChatColor.RED + "Invalid usage: /autoban <player> <reason>!");
                        return true;
                    }
                    if (args.length == 1) {
                        sender.sendMessage(ChatColor.RED + "Invalid usage: /autoban <player> <reason>!");
                        return true;
                    }
                    if (args.length >= 2) {
                        StringBuilder message = new StringBuilder("");
                        for (String part : args) {
                            if (!message.toString().equals(""))
                                message.append(" ");

                            message.append(part);
                        }
                        String reason = message.toString();
                        BanManager.getInstance().startAutoBan(args[0], reason);
                        return true;
                    }
                    return true;
                } else {
                    sender.sendMessage(
                            ChatColor.translateAlternateColorCodes('&', pl.getConfig().getString("No-Permission")));
                    return true;
                }
            }
            return true;

        }
    }
     
  2. You could just use the method 'cancel()'.
     
  3. Doesn't work, and I made my own method so I can remove the task from the map.
     
  4. It does in fact work, how exactly are you starting the task ?
     
  5. WAS

    WAS

    With this sort of setup, I suggest having a "Player" map, Map(Player, Map(String [reason], Integer [delay])) (or separate map with UUID -> Integer delay or w/e) reason, and than just don't run your code in the run() cycle if there is no one in the map. :D Just add a addPlayer() method.

    Than you just let the task run idle with nothing in it (no impact) as nothing runs. When a player shows up, and the map size is > 0 than execute your shabang.
     
    • Optimistic Optimistic x 1
  6. I would recommend you using Milliseconds instead of runnables for a Ban system, that way you won't have one instance of Runnable for each banned player. For better performance overall.
     
  7. WAS

    WAS

    If he used my suggestion it would just be one task started on server load.

    With milliseconds (timestamp) it would be reliant on the end player moving/doing something or something other task that runs regularly for the times to be compared. Which means tying into all sorts of events to be able to catch them. For example. You listen to movement (Wasting resources), sure. But what if they stand still and chat? You'd have to listen to the chat event too. Ultimately the time wouldn't be accurate.
     
  8. I don't get what he is trying to do. But if it's for bans, you'd have to listen to some Join/Login event to compare "timestamps" and that's it.
     
    • Agree Agree x 2
  9. WAS

    WAS

    It appears its a timed ban (i may be wrong). Like you ban someone and it waits a moment befor banning. So again, that wouldnt work.
     
  10. I dont see a cancel() method in the run()... all i see is return....
     
  11. Let's start with a few generic notes:
    • Instead of containsKey/get/remove, just call remove and do a null check (1 lookup instead of 3).
    • The activeBans field should be private, and use type Map rather than HashMap (Liskov).
      • If you need access to the Map, you can return an immutable view in a getter (through Collections::unmodifiableMap).
      • If you need to modify the Map from outside the class, write methods that deal with it.
    • Instead of put/get, use Map::compute(K, BiFunction<K, V, V>) and call runTaskTimer on the return value of compute.
    • String::replaceAll(String, String) is for regular expressions, which you don't seem to use. Use String::replace(String, String) instead.
    • You don't seem to check if the Ban-Command entry in the config is set.
    Now to get to your issue:
    • You override getTaskId(), don't (since BukkitRunnable uses it)
    • The issue is caused by this, since you set the taskId to getTaskId(), which returns the uninitialised taskId field (which is 0)
    • BukkitRunnable::cancel() uses getTaskId(), which you override incorrectly, hence it doesn't cancel the task.
    I personally dislike throwaway instances as well, as they might appear as unused when you read the code
    Code (Java):
    new AutoBanCommand(this);
    , so I'd personally recommend to just write this in your onEnable method.
     
  12. Thanks so much, I'll look into that! I don't really know why I made that taskId method, but I'll remove it.
     
  13. you could start your runnable like
    Code (Text):
    new BukkitRunnable {

    @Override
    public void run(){
    // your code


    //whenever you want put this cancel method:

    cancel();
    }

    }.runTaskTimer(plugin, 20, 20),
    Or something like that
     
  14. I know how they work, please read the thread.
     
  15. Actually, milliseconds would be the best option. All you'd need:


    // player joins
    // compare their timestamp assosiated with their account/id.
    // if System.currtenTimeMillis() > savedStamp = player is banned
     
  16. It's a runnable for a few of my own reasons, please don't try to give me other solutions (time stamps, etc) as they won't work with everything I want to do.
     
  17. WAS

    WAS

    Ugh. Again. It looks like its a auto ban system. Waits to ban the users in its list to give a global mesaage. Assuming the user is already online, what happens if they stand still, dont chat, etc? At least attempt to comprehend what the code intends to do.
     
    #18 WAS, May 4, 2017
    Last edited: May 4, 2017
  18. Code (Text):
     public BanTask(Main pl, String name, String reason) {
            this.id = getTaskId();
            this.pl = pl;
            this.name = name;
            this.reason = reason;
            delay = pl.getConfig().getInt("Delay");
        }

        public int getTaskId() {
            return id;
        }
    It appears that this might cause issues.. (Override-able method call in a constructor) - Id is not set, and it is told id = (id).

    If you're using the id to cancel the task, start the BukkitTask with the BukkitRunnable#runTask (whatever) methods and then set the id with a new method:
    Code (Text):
    setId(BukkitTask#getTaskId())
    https://hub.spigotmc.org/javadocs/spigot/org/bukkit/scheduler/BukkitRunnable.html
    https://hub.spigotmc.org/javadocs/spigot/org/bukkit/scheduler/BukkitTask.html#getTaskId()
     
  19. Sure. Have a scheduler. But the only purpose of this scheduler would be to check for people online and compare their Milliseconds stamp to current. And when banning, you often make a check to see if they're online, and if they are, kick them when adding them to the list with the milliseconds stamp.

    They don't have to do anything. When performing the autoban, just loop through the list and add their time stamp, additionally check if player is online, if they are kick, now they won't be able to join because we can use the JoinEvent or LoginEvent or whatever you want-- problem solved no scheduler