Skip to content
This repository was archived by the owner on Mar 9, 2022. It is now read-only.

Conversation

@Random-Liu
Copy link
Member

@Random-Liu Random-Liu commented Mar 27, 2018

Fix the event monitor panic.

We should only stop ticker when it is not nil.

Since start and stop can be called at the same time, we need a lock to protect it.

Signed-off-by: Lantao Liu [email protected]

@dmcgowan
Copy link
Member

LGTM

1 similar comment
@stevvooe
Copy link
Member

LGTM

Signed-off-by: Lantao Liu <[email protected]>
@Random-Liu Random-Liu force-pushed the fix-event-monitor-panic branch from b1a1dc9 to 277edb2 Compare March 27, 2018 01:41

type backOff struct {
queuePool map[string]*backOffQueue
queuePool map[string]*backOffQueue
Copy link
Member

Choose a reason for hiding this comment

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

go fmt

Copy link
Member Author

Choose a reason for hiding this comment

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

That is gofmt result.

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

See nit/comment.

defer b.tickerMu.Unlock()
if b.ticker != nil {
b.ticker.Stop()
}
Copy link
Member

Choose a reason for hiding this comment

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

else...logrus.WithError(err).Error("attempt to stop ticker before start has completed allocating the ticker")

note: should be impossible for this to happen now that you have a mutex... if the log is impossible we don't need the nil check..

Copy link
Member Author

Choose a reason for hiding this comment

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

This is possible, user may CTRL+C before CRI service starts.

Copy link
Member

@mikebrow mikebrow Mar 27, 2018

Choose a reason for hiding this comment

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

user interrupts ... interesting thx

return nil, errors.New("event channel is nil")
}
closeCh := make(chan struct{})
backOffCheckCh := em.backOff.start()
Copy link
Member

Choose a reason for hiding this comment

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

good!

@yanxuean
Copy link
Member

/lgtm

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

/LGTM

@Random-Liu Random-Liu merged commit 896e347 into containerd:master Mar 27, 2018
@Random-Liu Random-Liu deleted the fix-event-monitor-panic branch March 27, 2018 03:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants