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

Ability to register for events associated with jobs starting immediately #632

Merged
merged 3 commits into from
Dec 12, 2023
Merged

Conversation

drwpls
Copy link

@drwpls drwpls commented Dec 12, 2023

What does this do?

This commit add some functions to support register events for job which is on creating and run immediately.

Which issue(s) does this PR fix/relate to?

Job run immediately doesn't have before/after/onErr/onNoErr events.

List any changes that modify/break current functionality

No

Have you included tests for your changes?

Yes, it's in scheduler_test.go

Did you document any new/modified functionality?

Yes, in function header

Notes

Consider to change RegisterEventListeners to change only current job

Your Name added 3 commits December 12, 2023 18:09

Verified

This commit was signed with the committer’s verified signature.
etnbrd Etienne Brodu
This commit add some functions to support register events for job which
is on creating and run immediately.

Because of there is RegisterEventListeners in Scheduler receiver
interface (which will register event for all exsiting jobs), I don't
change the function.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
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.

This is a handy addition. Although, nothing in the current code prevents you from having event listeners on a job that starts immediately.

package main

import (
	"log"
	"time"

	"github.com/go-co-op/gocron"
)

func main() {
	s := gocron.NewScheduler(time.UTC)
	j, err := s.Every(1).Seconds().StartImmediately().Do(func() {})
	if err != nil {
		log.Fatal(err)
	}
	j.RegisterEventListeners(
		gocron.BeforeJobRuns(func(jobName string) {
			// do something
		}),
	)
	s.StartBlocking()
}

@JohnRoesler JohnRoesler merged commit 30921b7 into go-co-op:v1 Dec 12, 2023
@drwpls
Copy link
Author

drwpls commented Dec 13, 2023

This is a handy addition. Although, nothing in the current code prevents you from having event listeners on a job that starts immediately.

package main

import (
	"log"
	"time"

	"github.com/go-co-op/gocron"
)

func main() {
	s := gocron.NewScheduler(time.UTC)
	j, err := s.Every(1).Seconds().StartImmediately().Do(func() {})
	if err != nil {
		log.Fatal(err)
	}
	j.RegisterEventListeners(
		gocron.BeforeJobRuns(func(jobName string) {
			// do something
		}),
	)
	s.StartBlocking()
}

Your code will only work if the startBlocking statement runs after setting up the jobs, and it can't register events for individual jobs.

One more question: why did you define eventListenerFunc signature contain jobName (string) instead of *Job

@JohnRoesler
Copy link
Contributor

Your code will only work if the startBlocking statement runs after setting up the jobs, and it can't register events for individual jobs.

Correct, that you have to setup all the jobs first and then start. However, that does set the event listeners for an individual job. It's being called on the Job j.

why did you define eventListenerFunc signature contain jobName (string) instead of *Job

To avoid race conditions with the Job being a pointer.

Are there other fields that would be useful in the event listeners? I can definitely see how more info could be useful / better. I'd be happy to consider enhancements in v2 👍

@drwpls
Copy link
Author

drwpls commented Dec 13, 2023

If you set up 2 jobs, both got event registered. I don't think that is for individual job.
https://github.com/go-co-op/gocron/blob/30921b708f1ef7910842cc63fc27fe383c7e5d81/scheduler.go#L1517C1-L1528C2

Correct, that you have to setup all the jobs first and then start. However, that does set the event listeners for an individual job. It's being called on the Job j.

You already have mutex in job structure.

To avoid race conditions with the Job being a pointer.

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.

None yet

2 participants