Repository: go-co-op/gocron
Pull request #211 (2021-08-16 16:44:59)

Fix unique tag removal
### What does this do?

The remove functions have a bug where the unique tags set at the scheduler are only removed when using RemoveByTag which isn't used by the removal that occurs when the limit runs to is reached.

Updated the removal functions so that a check occurs to see if unique tags are being used and if so, remove all of the job's tags from the unique slice.

### Which issue(s) does this PR fix/relate to?
<!--- Put `Resolves #XXX` here to auto-close the issue that your PR fixes (if such) --->

resolves https://github.com/go-co-op/gocron/issues/210

### List any changes that modify/break current functionality


### Have you included tests for your changes?
👍 

### Did you document any new/modified functionality?

- [ ] Updated `example_test.go`
- [ ] Updated `README.md`

### Notes


djenriquez: Awesome! Was just running into this problem!

Patch:
diff --git a/.golangci.yaml b/.golangci.yaml
index 7568db3..611fb36 100644
--- a/.golangci.yaml
+++ b/.golangci.yaml
@@ -18,7 +18,7 @@ linters:
     - deadcode
     - errcheck
     - gofmt
-    - golint
+    - revive
     - gosec
     - gosimple
     - govet
diff --git a/job.go b/job.go
index 98cd924..613948b 100644
--- a/job.go
+++ b/job.go
@@ -195,10 +195,9 @@ func (j *Job) Weekdays() []time.Weekday {
 	return j.scheduledWeekday
 }
 
-// 	return j.scheduledWeekday
-// }
 // LimitRunsTo limits the number of executions of this job to n.
-// The job will remain in the scheduler.
+// Upon reaching the limit, the job is removed from the scheduler.
+//
 // Note: If a job is added to a running scheduler and this method is then used
 // you may see the job run more than the set limit as job is scheduled immediately
 // by default upon being added to the scheduler. It is recommended to use the
diff --git a/scheduler.go b/scheduler.go
index 21d7aa5..1d9244c 100644
--- a/scheduler.go
+++ b/scheduler.go
@@ -505,13 +505,17 @@ func (s *Scheduler) RunByTagWithDelay(tag string, d time.Duration) error {
 // The job function would need to have implemented a means of
 // stopping, e.g. using a context.WithCancel().
 func (s *Scheduler) Remove(job interface{}) {
+	fName := getFunctionName(job)
+	j := s.findJobByTaskName(fName)
+	s.removeJobsUniqueTags(j)
 	s.removeByCondition(func(someJob *Job) bool {
-		return someJob.name == getFunctionName(job)
+		return someJob.name == fName
 	})
 }
 
 // RemoveByReference removes specific Job by reference
 func (s *Scheduler) RemoveByReference(job *Job) {
+	s.removeJobsUniqueTags(job)
 	s.removeByCondition(func(someJob *Job) bool {
 		job.RLock()
 		defer job.RUnlock()
@@ -519,6 +523,26 @@ func (s *Scheduler) RemoveByReference(job *Job) {
 	})
 }
 
+func (s *Scheduler) findJobByTaskName(name string) *Job {
+	for _, job := range s.Jobs() {
+		if job.name == name {
+			return job
+		}
+	}
+	return nil
+}
+
+func (s *Scheduler) removeJobsUniqueTags(job *Job) {
+	if job == nil {
+		return
+	}
+	if s.tags != nil && len(job.tags) > 0 {
+		for _, tag := range job.tags {
+			delete(s.tags, tag)
+		}
+	}
+}
+
 func (s *Scheduler) removeByCondition(shouldRemove func(*Job) bool) {
 	retainedJobs := make([]*Job, 0)
 	for _, job := range s.Jobs() {
@@ -538,9 +562,6 @@ func (s *Scheduler) RemoveByTag(tag string) error {
 		return err
 	}
 
-	// Remove unique tag when exists
-	delete(s.tags, tag)
-
 	for _, job := range jobs {
 		s.RemoveByReference(job)
 	}
@@ -610,7 +631,7 @@ func (s *Scheduler) jobPresent(j *Job) bool {
 	return s._jobPresent(j, s.Jobs())
 }
 
-// Clear clear all Jobs from this scheduler
+// Clear clears all Jobs from this scheduler
 func (s *Scheduler) Clear() {
 	for _, job := range s.Jobs() {
 		job.stop()


Test patch:
diff --git a/scheduler_test.go b/scheduler_test.go
index 0cc3ba1..4701ced 100644
--- a/scheduler_test.go
+++ b/scheduler_test.go
@@ -328,7 +328,8 @@ func TestScheduler_Remove(t *testing.T) {
 
 	t.Run("remove from non-running", func(t *testing.T) {
 		s := NewScheduler(time.UTC)
-		_, err := s.Every(1).Minute().Do(task)
+		s.TagsUnique()
+		_, err := s.Every(1).Minute().Tag("tag1").Do(task)
 		require.NoError(t, err)
 		_, err = s.Every(1).Minute().Do(taskWithParams, 1, "hello")
 		require.NoError(t, err)
@@ -342,6 +343,8 @@ func TestScheduler_Remove(t *testing.T) {
 
 		s.Remove(task)
 		assert.Equal(t, 1, s.Len(), "Incorrect number of jobs after removing non-existent job")
+
+		assert.Zero(t, len(s.tags))
 	})
 
 	t.Run("remove from running scheduler", func(t *testing.T) {
@@ -380,9 +383,10 @@ func TestScheduler_RemoveByReference(t *testing.T) {
 
 	t.Run("remove from running scheduler", func(t *testing.T) {
 		s := NewScheduler(time.UTC)
+		s.TagsUnique()
 		semaphore := make(chan bool)
 
-		j, err := s.Every("100ms").StartAt(s.time.Now(s.location).Add(100 * time.Millisecond)).Do(func() {
+		j, err := s.Every("100ms").StartAt(s.time.Now(s.location).Add(100 * time.Millisecond)).Tag("tag1").Do(func() {
 			semaphore <- true
 		})
 		require.NoError(t, err)
@@ -397,6 +401,8 @@ func TestScheduler_RemoveByReference(t *testing.T) {
 		case <-semaphore:
 			t.Fatal("job ran after being removed")
 		}
+
+		assert.Zero(t, len(s.tags))
 	})
 }
 
@@ -835,6 +841,33 @@ func TestRunJobsWithLimit(t *testing.T) {
 			require.LessOrEqual(t, counter, 1)
 		}
 	})
+
+	t.Run("remove unique tags also", func(t *testing.T) {
+		semaphore := make(chan bool)
+
+		s := NewScheduler(time.UTC)
+		s.TagsUnique()
+
+		j, err := s.Every("100ms").Tag("tag1", "tag2", "tag3").Do(func() {
+			semaphore <- true
+		})
+		require.NoError(t, err)
+		j.LimitRunsTo(1)
+
+		s.StartAsync()
+		time.Sleep(200 * time.Millisecond)
+
+		var counter int
+		select {
+		case <-time.After(200 * time.Millisecond):
+			assert.Equal(t, 0, s.Len())
+		case <-semaphore:
+			counter++
+			require.LessOrEqual(t, counter, 1)
+		}
+
+		assert.Zero(t, len(s.tags))
+	})
 }
 
 func TestCalculateMonths(t *testing.T) {