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.

Sign up for GitHub

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

Fix inhibit race #1032

Merged
merged 2 commits into from
Oct 7, 2017
Merged

Fix inhibit race #1032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d50c12
inhibit: restructure stop logic, to prevent race condition
brancz Oct 5, 2017
c8945bd
vendor oklog/oklog/pkg/group
brancz Oct 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 37 additions & 18 deletions inhibit/inhibit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@
package inhibit

import (
"context"
"fmt"
"sync"
"time"

"github.com/oklog/oklog/pkg/group"
"github.com/prometheus/common/log"
"github.com/prometheus/common/model"

Expand All @@ -33,8 +35,8 @@ type Inhibitor struct {
rules []*InhibitRule
marker types.Marker

mtx sync.RWMutex
stopc chan struct{}
mtx sync.RWMutex
cancel func()
}

// NewInhibitor returns a new Inhibitor.
Expand All @@ -50,33 +52,26 @@ func NewInhibitor(ap provider.Alerts, rs []*config.InhibitRule, mk types.Marker)
return ih
}

func (ih *Inhibitor) runGC() {
func (ih *Inhibitor) runGC(ctx context.Context) {
for {
select {
case <-time.After(15 * time.Minute):
for _, r := range ih.rules {
r.gc()
}
case <-ih.stopc:
case <-ctx.Done():
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Think you should return ctx.Err()

Copy link
Member Author

Choose a reason for hiding this comment

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

ctx.Err() just returns why a context was canceled or deadline exceeded. It doesn't actually tell us anything here.

        // If Done is not yet closed, Err returns nil.
        // If Done is closed, Err returns a non-nil error explaining why:
        // Canceled if the context was canceled
        // or DeadlineExceeded if the context's deadline passed.
        // After Err returns a non-nil error, successive calls to Err return the same error.
        Err() error

Copy link
Contributor

Choose a reason for hiding this comment

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

true, but if non nil isn't worth logging?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so, because we explicitly use the context to cancel the execution in the Inhibitor.Stop method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the error is always != nil if the context was canceled. If we expect this to happen, there's generally no point in checking.

}
}
}

// Run the Inihibitor's background processing.
func (ih *Inhibitor) Run() {
ih.mtx.Lock()
ih.stopc = make(chan struct{})
ih.mtx.Unlock()

go ih.runGC()

func (ih *Inhibitor) run(ctx context.Context) {
it := ih.alerts.Subscribe()
defer it.Close()

for {
select {
case <-ih.stopc:
case <-ctx.Done():
return
case a := <-it.Next():
if err := it.Err(); err != nil {
Expand All @@ -98,17 +93,41 @@ func (ih *Inhibitor) Run() {
}
}

// Run the Inihibitor's background processing.
func (ih *Inhibitor) Run() {
var (
g group.Group
ctx context.Context
)

ctx, ih.cancel = context.WithCancel(context.Background())
gcCtx, gcCancel := context.WithCancel(ctx)
runCtx, runCancel := context.WithCancel(ctx)

g.Add(func() error {
ih.runGC(gcCtx)
return nil
}, func(err error) {
gcCancel()
})
g.Add(func() error {
ih.run(runCtx)
return nil
}, func(err error) {
runCancel()
})

g.Run()
}

// Stop the Inhibitor's background processing.
func (ih *Inhibitor) Stop() {
if ih == nil {
return
}
ih.mtx.Lock()
defer ih.mtx.Unlock()

if ih.stopc != nil {
close(ih.stopc)
ih.stopc = nil
if ih.cancel != nil {
ih.cancel()
}
}

Expand Down
201 changes: 201 additions & 0 deletions vendor/github.com/oklog/oklog/LICENSE
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

62 changes: 62 additions & 0 deletions vendor/github.com/oklog/oklog/pkg/group/group.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions vendor/vendor.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,12 @@
"revision": "db1efb556f84b25a0a13a04aad883943538ad2e0",
"revisionTime": "2017-01-25T05:19:37Z"
},
{
"checksumSHA1": "gkyBg/2hcIWR/8qGEeGVoHwOyfo=",
"path": "github.com/oklog/oklog/pkg/group",
"revision": "f857583a70c345341d679b3f27aa542c8db70a21",
"revisionTime": "2017-09-18T07:00:58Z"
},
{
"checksumSHA1": "8Y05Pz7onrQPcVWW6JStSsYRh6E=",
"path": "github.com/pelletier/go-buffruneio",
Expand Down