Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refact: schedules jobs with time.AfterFunc() #99

Merged
merged 7 commits into from
Dec 30, 2020

Conversation

Streppel
Copy link
Member

What does this do?

Main change: scheduler now uses time.AfterFunc from the standard time package to schedule jobs instead of manually sorting and looping through all jobs (yes, finally!)

This brings us:

  • assertion that our "run jobs" algorithm works without bugs as we're leaving this logic to a standard package
  • greater flexibility in what fields we want to keep in jobs - no need to actually keep other fields like nextRun or even our job slice for our run logic (they may still be useful for logging or getting meta info about a job or the scheduler, so they're not removed here)
  • minor performance gains as we don't sort and loop through our job array every second anymore
  • increased cohesion of our scheduler package

Unfortunately, after doing this refactor and separating the logic correctly, some tests broke, which made this pr a bit larger than what it needed to be. Fortunately, these failures actually brought minor errors to attention; I fixed them in here, so here's a list of what's different:

  • calling .Close() without listening to the channel returned by .StartAsync() would freeze the application; fixed by making stopChan a buffered channel: now the application doesn't freeze up anymore
  • before, the scheduler invoked each job by calling go job.Run(). This method not only calls the job function but also updates the run counter, which fails to remove jobs that start immediately (this passed our tests because all tests used .At()). This has been fixed, and now we first update our run counter and only later call the job's function; thus it's launch is actually the last thing to be done in the execution chain
  • refactored TestExecutionSecond to TestImmediateExecution, as this is what was actually being tested here
  • refactored TestAtFuture to use t.Run() and the new s.start() method
  • fixed TestRunJobsWithLimit to tests cases with immediate execution

List any changes that modify/break current functionality

None

Have you included tests for your changes?

Yes

Notes

I did my tests manually here, but if possible guys, take a look at some running examples to also check if they are working correctly.

A simple example of manual testing function that can be called from any testing file:

func TestManualTesting(t *testing.T) {
	s := NewScheduler(time.UTC)
	s.Every(1).Second().Do(func() {fmt.Println("job 1: ", time.Now().UTC().Format("15:04:05"))})
	s.Every(5).Second().Do(func() {fmt.Println("job 2: ", time.Now().UTC().Format("15:04:05"))})
	s.StartAsync()
	time.Sleep(11 * time.Second)
}```

j.setStartsImmediately(false)
}
if !j.shouldRun() {
if j.getRemoveAfterLastRun() { // TODO: this method seems unnecessary as we could always remove after the run cout has expired. Maybe remove this in the future?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

scheduler.go Outdated Show resolved Hide resolved
Copy link
Contributor

@JohnRoesler JohnRoesler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking awesome! just some races to clean up

JohnRoesler
JohnRoesler previously approved these changes Dec 28, 2020
@Streppel
Copy link
Member Author

Streppel commented Dec 28, 2020

@JohnRoesler thank you so much for the feedback and cleaning up the data race issues!

Edit: I just made one more commit as I mistakenly forgot to listen to s.stopChan, which is needed as we return the channel to the users and they may close it instead of calling the scheduler's .Stop() method. IMO we should stop returning it and make the scheduler stoppable by calling .Stop(), but that's another story

scheduler.go Outdated
for _, job := range s.runnableJobs() {
s.runAndReschedule(job) // we should handle this error somehow
func (s *Scheduler) run(job *Job) {
if !s.running {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnRoesler also, do you think this might help with that issue that we talked earlier on slack?

@Streppel Streppel merged commit 7eacf6d into master Dec 30, 2020
@Streppel Streppel deleted the refact/scheduler_uses_time_pkg branch December 30, 2020 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants