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 race condition when reloading configuration #32433

Merged
merged 1 commit into from
Jul 21, 2022
Merged

Fix race condition when reloading configuration #32433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions CHANGELOG.next.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ https://github.com/elastic/beats/compare/v8.2.0\...main[Check the HEAD diff]
- Expand fields in `decode_json_fields` if target is set. {issue}31712[31712] {pull}32010[32010]
- Fix OS name reported by add_host_metadata on Windows 11. {issue}30833[30833] {pull}32259[32259]
- Fix race condition when reloading runners {pull}32309[32309]
- Fix race condition when stopping runners {pull}32433[32433]

*Auditbeat*

Expand Down
4 changes: 3 additions & 1 deletion libbeat/autodiscover/autodiscover_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,7 @@ func TestAutodiscoverWithMutlipleEntries(t *testing.T) {
wait(t, func() bool { return len(adapter.Runners()) == 3 })
runners = adapter.Runners()
// Ensure the first config is the same as before
fmt.Println(runners)
t.Log(runners)
assert.Equal(t, len(runners), 3)
assert.Equal(t, len(autodiscover.configs["mock:foo"]), 2)
check(t, runners, conf.MustNewConfigFrom(map[string]interface{}{"a": "b"}), true, false)
Expand Down Expand Up @@ -571,6 +571,7 @@ func TestAutodiscoverWithMutlipleEntries(t *testing.T) {
}

func wait(t *testing.T, test func() bool) {
t.Helper()
sleep := 20 * time.Millisecond
ready := test()
for !ready && sleep < 10*time.Second {
Expand All @@ -585,6 +586,7 @@ func wait(t *testing.T, test func() bool) {
}

func check(t *testing.T, runners []*mockRunner, expected *conf.C, started, stopped bool) {
t.Helper()
for _, r := range runners {
if reflect.DeepEqual(expected, r.config) {
ok1 := assert.Equal(t, started, r.started)
Expand Down
4 changes: 2 additions & 2 deletions libbeat/cfgfile/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,11 @@ func (r *RunnerList) Reload(configs []*reload.ConfigWithMeta) error {
wg.Add(1)
r.logger.Debugf("Stopping runner: %s", runner)
delete(r.runners, hash)
go func() {
go func(runner Runner) {
defer wg.Done()
runner.Stop()
r.logger.Debugf("Runner: '%s' has stopped", runner)
}()
}(runner)
Copy link
Member

Choose a reason for hiding this comment

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

I wish the linter would have caught this...

Am I correct that this means some running modules will never actually be stopped depending on the timing? This definitely feels like something we should include in the 8.3.3 release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wish the linter would have caught this...

Am I correct that this means some running modules will never actually be stopped depending on the timing? This definitely feels like something we should include in the 8.3.3 release.

Yes, it does. Without this PR, the situation is worse then the original bug 😢

But the original buggy PR is very recent, so I strongly believe no releases were made in the mean time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wish the linter would have caught this...

Me too, there should be a linter for that. It's such a basic case of variable shadowing...

Copy link
Member

Choose a reason for hiding this comment

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

Go vet can do it but it is disabled by default: https://golangci-lint.run/usage/linters/#govet

Probably it will be noisy but I can try turning it on and see what result it gives me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to run go vet but for some reason it didn't detect it. I even tried go vet -loopclosure, still not error shown :/

But I didn't spend much time trying to get it working

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to run go vet but for some reason it didn't detect it. I even tried go vet -loopclosure, still not error shown :/

https://cs.opensource.google/go/x/tools/+/refs/tags/v0.1.7:go/analysis/passes/loopclosure/loopclosure.go;l=24;bpv=1 (thanks to gopher slack tools people).

moduleStops.Add(1)
}

Expand Down