Why does BukkitRunnable.cancel() not set taskId back to -1 ???

Discussion in 'Plugin Development' started by Weasel_Squeezer, Apr 12, 2014.

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

    Weasel_Squeezer

    I have run into a problem where I want to have a BukkitRunnable task cancel itself and reschedule itself, but BukkitRunnable makes it impossible to do this since it will throw an IllegalStateException if the task ID is not -1 (meaning it was already scheduled). But when you cancel the task, it should set it back to -1 since it is no longer running, or in the qeue, and thus is no longer scheduled. Look at the source for BukkitRunnable:
    https://github.com/Bukkit/Bukkit/blob/master/src/main/java/org/bukkit/scheduler/BukkitRunnable.java
    Code:java
    1. public synchronized void cancel() throws IllegalStateException {
    2. Bukkit.getScheduler().cancelTask(getTaskId());
    3. }
    4.  
    5. public synchronized BukkitTask runTaskTimer(Plugin plugin, long delay, long period) throws IllegalArgumentException, IllegalStateException {
    6. checkState();
    7. return setupId(Bukkit.getScheduler().runTaskTimer(plugin, this, delay, period));
    8. }
    9.  
    10. public synchronized int getTaskId() throws IllegalStateException {
    11. final int id = taskId;
    12. if (id == -1) {
    13. throw new IllegalStateException("Not scheduled yet");
    14. }
    15. return id;
    16. }
    17.  
    18. private void checkState() {
    19. if (taskId != -1) {
    20. throw new IllegalStateException("Already scheduled as " + taskId);
    21. }
    22. }
    23.  
    24. private BukkitTask setupId(final BukkitTask task) {
    25. this.taskId = task.getTaskId();
    26. return task;
    27. }

    As you can see, Cancel() only cancels this task from the scheduler, and does not update the taskId. So this makes it impossible to do something like:
    Code:java
    1. cancel();
    2. runTaskTimer(plugin, 0, MINUTE_IN_TICKS);


    What is up with this? Is there a reason for this? Am I missing something?
     
  2. Offline

    Backfighter

    I would be interested in an answer too. :cool:
    As it's impossible to make the runnable instance itself again to reset the id did you find another way sloveing this problem?
     
  3. Offline

    Europia79

    @Weasel_Squeezer

    Good question. The BukkitRunnable is supposed to exist for convenience... So that you don't have to manually schedule a Runnable with Bukkit.

    You see... if you do the opposite (you don't use a BukkitRunnable), then when you schedule a task, you'll have to save the taskID somewhere... Whereas, if you use a BukkitRunnable, then these implementation details are hidden: It becomes a more object orient approach where the taskID is now an internal attribute of BukkitRunnable objects... And those objects have behaviors of runTaskX() and cancel()

    The problem (as you observed) is that the behavior of cancel() isn't what is expected. Luckily, it does warn you with an IllegalStateException.

    I'm speculating that the designers wanted a quasi-immutable object to protect the end-user: Think about it... A typical extension of BukkitRunnable will have its own state (fields). If you were allowed to re-use it (continually start, cancel, start, cancel, start...), would you remember to reset your own fields ? (either before a start or after a cancel).

    i think the designers could have chosen a better name to make the behavior of BukkitRunnable::cancel() more apparent. I think a better name would have been OnceRunnable (to imply that it can only be run once).

    On top of that, let's take a look at the documentation:
    "tasks" plural should be change to "a task." singular. More info should be added to the documentation to make it perfectly clear: "The BukkitRunnable can only be used once: Meaning you cannot call runTaskX() after you've called cancel()."

    @Backfighter

    There are two solutions here:

    1. Keep using the BukkitRunnable... Realize it's true behavior (as a OnceRunnable), and tweak your design to accommodate this behavior: Meaning that you'll have to create a new object for every task that you want to run.

    Code:java
    1. bukkitRunnableField = new SomeBukkitRunnable(this);
    2. bukkitRunnableField.runTaskTimer(plugin, 0L, 20L);
    3. // ...
    4. bukkitRunnableField.cancel();
    5. //...
    6. bukkitRunnableField = new SomeBukkitRunnable(this);
    7. bukkitRunnableField.runTaskTimer(plugin, 0L, 20L);


    2. Create a new class that behaves differently: A ReRunnable (so to speak) that resets the interal taskID back to -1 on cancel()... which would allow runTaskX() to be called again.

    Code:java
    1. public abstract class ReRunnable implements Runnable
    2. {
    3. private int taskId = -1;
    4. public synchronized void cancel() throws IllegalStateException
    5. {
    6. Bukkit.getScheduler().cancelTask(getTaskId());
    7. this.taskId = -1;
    8. }
    9. // ...
    10. }

    Or if you wanted your ReRunnable class to extend BukkitRunnable, you could override cancel(), but since taskID is private, you'd have to use reflection to change the value.
     
    Avygeil and Rocoty like this.
  4. Maybe this works .
    Code:java
    1.  
    2. private BukkitTask task;
    3. public void runnable()
    4. {
    5. this.task = new BukkitRunnable()
    6. {
    7. @Override
    8. public void run() {
    9. //code
    10. }
    11. }.runTaskTimer(plugin, 10, 10);
    12. }
    13. public void cancel(Player player) {
    14. task.cancel();
    15. }
     
  5. Offline

    mythbusterma

    @Europia79 @Weasel_Squeezer

    Alternatively, the BukkitScheduler allows you to submit classes that only implement Runnable, so you could just have a class that does not extend BukkitRunnable, and therefore would not have an internal task ID.

    You could use an external class to get the BukkitTask that is returned from scheduling this, and invoke cancel() when necessary, and then reschedule the same Runnbale at a given delay, similar to what Europia had, but you don't even need to bother with the taskID or a cancel() method, just repeatedly submit the same Runnable.

    Oh, and as usual, don't listen to @MaTaMoR_
     
  6. Offline

    1Rogue

    You can also use a box to access the returned task from within the runnable:

    Code:java
    1. public class Box<T> {
    2.  
    3. private final volatile T item;
    4.  
    5. public Box(T item) {
    6. this.item = item;
    7. }
    8.  
    9. public T get() {
    10. return this.item;
    11. }
    12.  
    13. }

    Code:java
    1. Box<BukkitTask> task = getServer().getScheduler().runTaskTimer(() -> {
    2. task.get().cancel();
    3. }, /* other params */);


    And if you have an already established class instead of an anonymous runnable, you can just use a private abstract method for cancelling:
    Code:java
    1. public class MyRunnable implements Runnable {
    2.  
    3. public void run() {
    4. //...
    5. this.cancel();
    6. }
    7.  
    8. protected abstract void cancel();
    9.  
    10. }

    Code:java
    1. Box<BukkitTask> task = getServer().getScheduler().runTaskTimer(new MyRunnable() {
    2. public void cancel() { task.get().cancel(); }
    3. }, /* other params */);


    (Would be a good place for your own interface that extends Runnable for your Runnables).

    Since bukkit's (and most) scheduler implementations work as:
    Code:
    Register runnable > return task > execute runnable
    This works in a hacky way.
     
    mythbusterma likes this.
  7. Offline

    xTrollxDudex

    @1Rogue
    Why is "item" volatile? It should be final in this instance.
     
    mythbusterma likes this.
  8. Offline

    1Rogue

    It is both volatile and final. Possibly unnecessary now since I set the variable from the constructor but I'm used to other Box implementations where:

    Code:java
    1. public class Box<T> {
    2. public volatile T value;
    3. }


    But it helps me sleep at night.
     
Thread Status:
Not open for further replies.

Share This Page