Solved Repeating Task Sometimes Breaks

So I'm trying to make a repeating task that would run something every 15 minutes. It usually works however now and then it will break and run what needs to be run every 15 minutes repeatedly. Here's the code.

Code:

 

Do you know when it specifically breaks? And do you have an error from when it breaks? Also, judging from the timeMax config value, it's every 30 minutes, not 15. 15 minutes would be 900 seconds.

Also, I put this together for the Bukkit task, as it is easier to understand from this (and removes the same functionality and stacks it into one line):

Code:

Bukkit.getScheduler().scheduleAsyncRepeatingTask(plugin, new Runnable() {
    public void run() {
     
        FileConfiguration config = null;
        File file = new File("plugins" + File.separator + "PayDay" + File.separator + "users" + File.separator + player.getUniqueId() + " (" + player.getName() + ")" + ".yml");
        config = YamlConfiguration.loadConfiguration(file);

        int timeLeft = config.getInt("Player.Time-Left"); //Consideration: Cache these values in a hashmap, then when the player logs off, save their final value
        int timeMax = config.getInt("Player.Time-Max");
        String groupName = PermissionsEx.getUser(player).getGroups()[0].getName();
     
        if(config.getInt("Player.Time-Left") <= 0) {
            if(groupName.equalsIgnoreCase("Peasant") || groupName.equalsIgnoreCase("Serf") || groupName.equalsIgnoreCase("Merchant")
            || groupName.equalsIgnoreCase("Knight") || groupName.equalsIgnoreCase("Noble") || groupName.equalsIgnoreCase("King") 
            || groupName.equalsIgnoreCase("Demigod") || groupName.equalsIgnoreCase("Immortal")) {
                Bukkit.getServer().dispatchCommand(Bukkit.getConsoleSender(), "eco give " + player.getName() + " " + amount);
                player.sendMessage(prefix + "You have recieved your paycheck of " + ChatColor.GREEN + "$" + amount);
                config.set("Player.Time-Left", timeMax);
                try {
                    config.save(file);
                } catch (IOException e) {
                }
            }
        } else {
            if(player.isOnline()) {
                config.set("Player.Time-Left", --timeLeft);
                try {
                    config.save(file);
                } catch (IOException e) {
                }
            } else {
                return;
            }
        }
    }
}, 20, 20);

 

Thanks for the speedy response and fixed up code. From what I remember, there was no error in the console. I don't know why it breaks. What basically happens is randomly when it gets to 0 (the countdown) it doesn't reset back to 0 and it continuously pays me until I restart the server and delete the player's config while its reloading and reconnect.
EDIT:
Also I've noticed that the server has gotten fairly laggy with this. It may no be from this but its very likely from it.

 

@Drkmaster83
Please don't spoonfeed. If you have to, write psudo code. Writing actual code that is untested and may not work may create more problems for both the OP and anyone reading this thread in the future.

@plisov
Do you have any code checking if the countdown is less than or equal to 0, and if it is, setting it back to the number you want (since setting it back to 0 will just create a continuous loop.)

 

He didn't ask for the code, and I hardly spoonfed him. Even then, the problem with pseudo code is that it can be misunderstood just as easily as writing it out. The only thing I did in the code I provided was take the duplicate functions and merge the if statements to lead to the same block of statements.

Worst case, he can post his updated code. So long as progress is made, or a new issue arises, it keeps things moving.

@plisov Odd that it won't reset the time-left back to the max time value, it would definitely benefit you to put in the printStackTrace for the errors, as those could be helpful to see if it's an IO exception that's breaking the task.
Also, are you testing it by yourself when it happens randomly? I'll definitely test this myself and see what happens

 

Alright, but keep in mind it won't give you the error at once.
@Zombie_Striker Yes, I'm checking if it's less than or equal to 0. I want it to loop so that it runs every 15 minutes.

 

I think I've got it, @plisov! Your issue is likely stemming from the fact that you're using a repeating task for the player and then not cancelling it when they log off. It's not the deleting of the player's file in the userdata folder that is fixing your issue, it's the reloading of the server (as this cancels all tasks that are running). I did encounter and reproduce the issue you were talking about. I noticed that in the config, it would somehow delete the Time-Max setting, which would then end up having the time-left set to 0 (since if config.getInt() returns null, it results in the default int value, 0), making it pay you every second that the task repeats. I bet it's something to do with the two tasks accessing the same file (potentially in the same instant), but once you cancel the task (where the else is when the player ISN'T online), it won't matter anymore!

 

Ah! This is most definitely the problem. However if I cancel the task, wouldn't it cancel it for everyone? Also how would I cancel it?

 

Therein lies the problem. The way you've scheduled the task makes it a bit difficult to cancel, but if you change

Code:

Bukkit.getScheduler().scheduleAsyncRepeatingTask(plugin, new Runnable() {
//and at the bottom of the runnable
}, 20, 20);

to

Code:

new BukkitRunnable() {
//and at the bottom of the runnable
}.runTaskTimer(this, 20, 20);

What this accomplishes is giving you the ability to type

Code:

this.cancel();

inside the runnable (thus cancelling it) without having to store its task ID (gets really complicated, uses more RAM).
Additionally, a bonus of this is that you no longer run into deprecation, as BukkitRunnables are supported.

 

Ah. Thanks for that. So this would be more or less correct?

Code:

Wouldn't this cancel the bukkit runnable for everyone on the server if someone leaves though?

 

You've put the cancel in the wrong place, that'll cancel it on the first run when the timeLeft is greater than 0 (resulting in everyone never getting any money ever). The Solution? Put it in the else (you can even replace the return statement with the this.cancel()).

As to the second part of your post, no, it would not cancel the bukkit runnable for everyone, as a new bukkit runnable is created for every single person that joins. There would be a way to make this a global runnable, and that might be more efficient, but at the end of the day, it's up to you as to what you want to use.

Global runnable example (put in onEnable, never an event, don't need several global - meaning all players - runnables running at the same time):

Code:

public void onEnable() {
    new BukkitRunnable() {
        @Override
        public void run() {
            for(Player p : getServer().getOnlinePlayers()) { //loop through all online players
                //do the normal jail code, using "p" as a player
            }
        }
    }
}.runTaskTimer(this, 20, 20);

Note that with the global version, the PlayerJoinEvents and PlayerQuitEvents would be necessary.

onPlayerJoin (per-player) task:

Code:

what you've got.

 

Oh, oops. Too tired sorry. Put it in the wrong spot. So you think this may fix our problem where the counter doesn't reset sometimes? Thanks by the way for taking me through the process.

EDIT: Another thing I've noticed is that the time is counting down way to slow. I left and it was at 10 min and like 5 minutes later I come back and it's at 8 min. I think I may be able to fix that. It's probably the 1800. I'm dividing that number by another number to make it say 15 when I run a command. I'll try to fix it.

 

@plisov, Ideally, yes. The problem wasn't that the counter wouldn't reset, but rather a glitch with two tasks doing the same thing at the same time.

This erased TimeMax in the player's config, and when you try to get something that doesn't exist for an int, it returns 0.

Code:

int i;
System.out.println(i);

The console would print out "0", even though you didn't set i to 0.
Continuing on, since timeLeft gets reset to timeMax when timeLeft is 0, and since timeMax is, timeLeft is infinitely 0, which leads to the constant payday every second.

 

Alright. So the countdown seems to be working. I went afk for the night and everything was going great. However the amount you get paid started off with the correct amount which was 250 and then changed to 100.

EDIT: I've also noticed that the timer goes a bit slow. Here is my updated code.

Code:

 

Your amount variable is of global scope. Move it into the player join method above the if statements. When another player joins the server, it'll change the amount for everyone to their value.

As for the slowness, I haven't any idea. Maybe server lag?

 

But if I move it I won't be able to access it when I'm setting amount received in config and when sending the message of how much a player was paid.

 

I'll say this much. You will, because if you move it into the Player join event, it'll be recreated every time for every player that joins (same concept as the repeating task). I'll definitely explain this further if you need, so don't be shy.

 

Ah. So you're saying that if I move everything outside of the playerjoinevent into the playerjoinevent it'll work? I can't access my code at the moment because I tried reinstalling java and now Eclipse won't open.

EDIT: I've gotten eclipse to open. Not to worry

 

So, basically, the code can see variables depending on where they are. The easiest way is to think of this idea is the indentation up until a certain space and then more indented is visible.

Code:

int amount;

public void onPlayerJoin () {
    amount = 1;
}

vs.

Code:

public void onPlayerJoin () {
    int amount;
    amount = 1;
}

The first one keeps that value after the method is done. The second one creates it to use it and then throws it away after the method is done to have it get picked up by Java's garbage collector (helps keep ram usage down). The first one would be helpful for values all players have, like default TimeMax, but Time-left changes for all players, so it can't be for all of them. This concept is called global scope vs local scope, you can Google it or just ask me.

Two examples of what's wrong:

Code:

public void onPlayerJoin() {
    amount = 0; //Not declared yet, will error out because the code doesn't know what type "amount" is (int, string, etc)

    int amount; //the declaration, specific to onPlayerJoin, not the whole class. This is out of order though, because we refer to it prior to creating it
}

and

Code:

public void onEnable () {
    amount = 0; //onEnable doesn't know about the value that was created in onPlayerJoin, so this errors out
}

public void onPlayerJoin () {
    int amount = 0; //specific to onPlayerJoin method
}

To answer you fully, I'm saying move the int amount; from the top right above where you check all their ranks and set that value. You'll know you did it right when it's inside the on Player join method and has no errors.

 

I think I kind of get it but not really. I have it like this and this way has no errors.

 

@plisov For the first part, before the edit: so, do it like you did in Code #2, except add the word 'final' before it. Why is this necessary? The BukkitRunnable can't have the value change, it needs to know that that will be the final value.

As for the second part (the issue still occurring), that has to do with something else other than this. I'd need the Enable class, because with this code right now, if you reload the server while people are still on, it should never send them any more money until they relogged.

 

I've tried adding final previously but it still didn't fix the problem to I removed it. I put final int amount; in the onPlayerJoinEvent. I only assume now that I should take the final int amount; and place it outside of the event, but that would cause more errors. Seeing how I need the amount variable to change, I don't really know why I'm setting it to final.

My enable class is literally just a command showing how much time is left. I have an onMove class that checks to see if their rank changed, and if it did set the new values in their config. Here it is.

Code:

 

Well, as the plugin is now, I couldn't recreate the bug. I'll try plugging in the onMove, but for now, I can't see why that occurs.

 

Any ideas? @timtower @Zombie_Striker

 

I hate this problem not being sloved, so if you'd like, I will join your server and observe, even voice chat with you if necessary. I can't replicate the issue (although I didn't leave it overnight), so surely it must be something specific to your environment.

Also,

I hate to hear that, so allow me to try to clarify; any Runnable or time-dependent part of Java (even Threads, to a degree) need reliability. In the BukkitRunnable, the server is storing it as an object that alters it's own value (an enclosed scope object), therefore, any value that comes from an external source (i.e, your amount, as it comes from your on Player join method) needs to never be able to change value. Hence, we add the keyword 'final' to assure the runnable that we won't (and can't) change it anymore. A final value can only be set once, and this is why it's perfect.
It is also important to note that technically, it doesn't change, but rather varies from player to player. Changing would imply that we reset it, but in reality, we are recreating it for each player that logs on.

 

Sure. So when I join the server, the player's counter starts working. Once it hits 0 it gives the player money and then resets the counters. Randomly the amount the player gets paid changes to either 50 or 100. When I reload the server, the counter for only one person (as far as I can tell) breaks causing them to get spammed with payments every second or so. Here is the up-to-date code.

Code:

 

Hmmm. How would I make them local?