michaeluno / Task-Scheduler

Provides a task management system for WordPress
Other
14 stars 7 forks source link

Scheduling glitch #11

Closed GutHib closed 7 years ago

GutHib commented 7 years ago

Problem: When task is supposed to run 7 days from current date, it is scheduled to run a day too early.

Solution: In in TaskScheduler_Occurrence_Daily.php the function _getNextClosestTime() removes the current day from the day-of-the-week array. In the next step, _getNumberOfDaysToClosestDay() can’t find the correct weekday (weekday a week ahead is the same weekday as today, which is missing now) and returns the default “6” in line 231. Changing that value to “7” fixes the problem.

michaeluno commented 7 years ago

Thanks for the report.

To clarify, this is for the Daily occurrence type.

The problem seems to occur when the user sets a time earlier than the current time of the day. For example, say, the user creates a task with the Daily occurrence type with 8:00 and Monday checked, when his/her current time is at 7:00 on Monday, the plugin sets the time correctly. However, when the user's time is at 9:00 on Monday, it fails.

Can you check if changing the return value to 7 does not break anything on the both scenario described above?

GutHib commented 7 years ago

First of all, you're probably asking the wrong guy. I'm not even a half decent programmer - I simply poke around in code. Secondly, it's your plugin - of the things it's supposed to do, I only understand whether or not it produces the results I need, so please don't ask me if it something breaks anything else - I simply wouldn't know.

What I do know is that the above changes didn't fix the issue for me. Entire _getNumberOfDaysToClosestDay function now looks like this:

            private function _getNumberOfDaysToClosestDay( $iSubjectDay, array $aDays ) {

                // First loop 
                // $_iDay is 1 to 7, Mon to Sun
                $_iDistanceDays = 0;
                for ( $_iDay = $iSubjectDay; $_iDay <= 7; $_iDay++ ) {
                    if ( in_array( $_iDay, $aDays ) ) {
                        return $_iDistanceDays;
                    }
                    $_iDistanceDays++;
                }
                // Second loop
                for ( $_iDay = 1; $_iDay <= $iSubjectDay; $_iDay++ ) {
                    $_iDistanceDays++;
                    if ( in_array( $_iDay, $aDays ) ) {
                        return $_iDistanceDays;
                    }
                }

                // Not found. Returns the number of days of one week (zero-based).
                return 7;

            }

Seems to work fine now, but yesterday's poking around in the code also produced 100+ automated instances of the same post, so we will see.

To answer your other questions:

GutHib commented 7 years ago

UPDATE. Seems like there are two glitches:

1 (as stated previously) the default value in line 231 needs to be set to 7 (instead of 6).

2 In _getNextClosestTime() you write you want to find the "timestamp of the closest date from the current time". But instead of working with the current time, you're actually using the task's last runtime. Why? I replaced that with time() in line 184 and now it seems like everything is in order.

michaeluno commented 7 years ago

But instead of working with the current time, you're actually using the task's last runtime. Why?

That function also gets called when a task with the Daily occurrence type is finished and in that case, the last run time is used. For cases that the user creates a new task, the last run time is not set. In that case, _formatLastRunTime() gives the current time with time().

Maybe are you updating the task (via Dashboard -> Task Scheduler -> Tasks -> Edit)? If so, the last run time may exist thus it could be causing a problem.

GutHib commented 7 years ago

Yes, there's where I am updating it. Would there be an alternative way to do it?

On the flipside, there seems no disadvantage in using time() instead of last runtime, because when a task with the daily occurrence type is finished, the two are identical anyway.

michaeluno commented 7 years ago

Would there be an alternative way to do it?

There are mainly three scenarios that call the function to calculates the next run time.

  1. The user creates a new task.
  2. The user edits a task.
  3. The task completes and schedules the next routine.

On the flipside, there seems no disadvantage in using time() instead of last runtime, because when a task with the daily occurrence type is finished, the two are identical anyway.

Okay, I don't see or predict any side effects by changing it to time() from the last run time. Thanks for suggesting that.

I've uploaded a development version (v1.4.5b01) which includes the changes discussed here so you can try it. And please see if you get any further issues.

GutHib commented 7 years ago

It's not working. Was supposed to run at 7 am this morning, ran at 8:47 am instead (according to "last run" information). Didn't actually post anything, though.

Clicking on "run now" works fine, but that somehow defeats the purpose of the plugin.

GutHib commented 7 years ago

Couple of questions:

Thanks

michaeluno commented 7 years ago

Hey, sounds like you have a different issue. Can you create a new topic about it? Thank you.

To answer you questions briefly.

  1. Yes, I wrote it from scratch.
  2. If the server heartbeat is turned off, scheduled tasks will not be executed on time.
  3. I haven't heard any.
GutHib commented 7 years ago

"Yes, I wrote it from scratch."

I am asking because you seem extremely passive in this. Like the plugin wasn't yours to begin with. Basically you are just sitting there waiting for me to come up with solutions. Which of course you have every right to do - I just find it extremely unusual. Most people who spend so much work to write a plugin for the common good (fantastic!) are very eager to squash the final bugs and don't stop on the last yard.

"If the server heartbeat is turned off, scheduled tasks will not be executed on time."

Well, it was turned on, and scheduled tasks weren't executed in time. So, what do you recommend?

michaeluno commented 7 years ago

I just find it extremely unusual.

If you feel unusual, maybe you must be still new to the internet and open source projects. I wrote it a couple years ago and my focus has shifted. I'm trying to maintain it at best while doing other things.

Well, it was turned on, and scheduled tasks weren't executed in time. So, what do you recommend?

You should create a test task by scheduling with a very near time such as 2 minutes ahead. Than see if it executes in time. If it does, make the time farther little by little. At some point, the problem should appear. Then you can tell in what cases, it happens. Then, others will be able to reproduce it. If others can reproduce it, there will be more chances for the problem to be fixed.

By the way, WordPress 4.8 has been released recently. So it is possible that there are incompatibility issues that we are not aware of.