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

Fix data race in mockbeat (#5037) #5426

Merged
merged 1 commit into from
Oct 24, 2017
Merged

Conversation

adriansr
Copy link
Contributor

Mockbeat expects Beater.Stop to be called after Beater.Run has
initialized some shared state. This is a race as Stop can be called at
any moment, even before Run is ever called.

This caused race warnings and segfaults when running unit tests.

Closes #5037

@urso
Copy link

urso commented Oct 24, 2017

This is a general problem all beats currently have. The Stop method might need to cleanup/close state/variables that are only created during the run method. Reason Stop closes the client is, if the client is blocking the beat will never stop.

+100 If this PR really solves some issues when using -race.

But longterm I'd like to have some support to signal shutdown in the run method, potentially removing the Stop method. e.g. think context.Context. Or something more tailored like:

type Signal struct { ... }

func (s *Signal) C() <-chan struct{} { ... }

func (s *Signal) CloseOnSig(c io.Closer) { ... }
func (s *Signal) CBOnSig(fn func()) { ... }

Copy link
Contributor

@tsg tsg left a comment

Choose a reason for hiding this comment

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

LGTM, nice work! The test failures are because of the sha512 files, they should go away if you rebase to master.

@adriansr adriansr added the in progress Pull request is currently in progress. label Oct 24, 2017
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM

@adriansr adriansr removed the in progress Pull request is currently in progress. label Oct 24, 2017
Mockbeat expects Beater.Stop to be called after Beater.Run has
initialized some shared state. This is a race as Stop can be called at
any moment, even before Run is ever called.

This caused race warnings and segfaults when running unit tests.

Closes elastic#5037
@andrewkroh andrewkroh merged commit d1dfc7e into elastic:master Oct 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants