Rules manager Panic caused by closing a channel more than once.

[krasi-georgiev]

caused when sending a SIGHUP. and SIGTERM right after that.
I looked briefly and seems like an easy one to solve.

panic: close of closed channel

goroutine 86182 [running]:
github.com/prometheus/prometheus/rules.(*Group).stop(...)
      ..../src/github.com/prometheus/prometheus/rules/manager.go:238
github.com/prometheus/prometheus/rules.(*Manager).Update.func1(0xc58eeacc01, 0xc42842c480, 0xc4200c5bc0, 0xc53ad459e0, 0xc44f874a20)
      ..../src/github.com/prometheus/prometheus/rules/manager.go:501 +0x80
created by github.com/prometheus/prometheus/rules.(*Manager).Update
      ..../src/github.com/prometheus/prometheus/rules/manager.go:499 +0x512
panic: close of closed channel

goroutine 86140 [running]:
github.com/prometheus/prometheus/rules.(*Group).stop(...)
      ..../src/github.com/prometheus/prometheus/rules/manager.go:238
github.com/prometheus/prometheus/rules.(*Manager).Update.func1(0xc4fab69b01, 0xc428228b40, 0xc4200c5bc0, 0xc53ad459e0, 0xc4e381def0)
      ..../src/github.com/prometheus/prometheus/rules/manager.go:501 +0x80
created by github.com/prometheus/prometheus/rules.(*Manager).Update
      ..../src/github.com/prometheus/prometheus/rules/manager.go:499 +0x512

[daveworth]

I would love to take on fixing this bug.

[krasi-georgiev]

the report was from @beorn7 and I already opened a PR(link above) with a suggested fix , but that is without managing to replicate it.
It would help if you can manage to replicate it and check if the PR fixes the issue.

[daveworth]

👍 Sorry I missed that PR @krasi-georgiev - I will attempt to replicate and post results.

[krasi-georgiev]

thanks

[krasi-georgiev]

@daveworth you can have a go if you want as I couldn't replicate it and a better PR would be finding what caused closing this channel twice rather than preventing for double closing the same channel.

[brian-brazil]

I think this race is more likely to happen the other way, TERM and then HUP.

[daveworth]

👋 sorry for my delay - job transitions and travel interrupted my work. Back on it.

[codwu]

I have thousands of rules in my Prometheus. It is easy to replicate it when sending SIGTERM.

[bboreham]

Here's a test that gives "close of closed channel", using the fixture yaml from #4306 :

func TestStop(t *testing.T) {
	files := []string{"fixtures/rules.yaml"}
	ruleManager := NewManager(&ManagerOptions{
		Context: context.Background(),
		Logger:  log.NewNopLogger(),
	})
	ruleManager.Run()

	err := ruleManager.Update(10*time.Second, files)
	testutil.Ok(t, err)

	ruleManager.Stop()

	err = ruleManager.Update(10*time.Second, files)
	testutil.Ok(t, err)
}

What is the expected state of Manager after Stop() ? Blanking groups would be an easy fix.

[brian-brazil]

Not panicing would be the expected state :) It should no longer be writing anything to storage.

Emptying groups was also my thought on handling this.

[bboreham]

Given that Stop() is only called from the teardown handler in main(), I venture to suggest that it should put Manager into a state where subsequent Update() calls do not start any further work.

[brian-brazil]

The other option is that we make it so an Update can't be called after a shutdown is initiated. It might be better to centralise that logic in once place rather than spreading it around in each subsystem.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.