Skip to content

UI's ConfigProviderWithRefresh enters tight loop after Close is called #3001

@bpalmer-lyft

Description

@bpalmer-lyft

Describe the bug

In server/server/config/config_provider_with_refresh.go , refreshConfig enters a tight loop
after the Close() method is called, because break only breaks out of select, rather than the entire for loop. If it used return instead, or a labelled break, it would exit immediately.

That is, in refreshConfig,

	for {
		select {
		case <-s.stop:
			break
		case <-s.ticker.C:
		}

		newConfig, err := s.provider.GetConfig()
                 // ...

should probably instead be

	for {
		select {
		case <-s.stop:
			return
		case <-s.ticker.C:
		}

		newConfig, err := s.provider.GetConfig()
                 // ...

To Reproduce
The bug can be seen in a test similar to

type TestConfigProvider struct {
	callCount int
}

func (tcp *TestConfigProvider) GetConfig() (*Config, error) {
	tcp.callCount++
	return &Config{
		RefreshInterval: time.Second,
	}, nil
}

func TestStopping(t *testing.T) {
	provider := new(TestConfigProvider)
	start := time.Now()
	refresh, err := NewConfigProviderWithRefresh(provider)
	if err != nil {
		t.Fatalf("unexpected error: %s", err)
	}
	if provider.callCount != 1 {
		t.Fatalf("Expected refresh to happen once but it occurred %d times", provider.callCount)
	}
	refresh.Close()
	seconds := time.Since(start).Seconds()
	if provider.callCount != 1+int(seconds) {
		t.Fatalf("Expected refresh to happen %d times but it occurred %d times", 1+int(seconds), provider.callCount)
	}
}

(You'll also see a lot of loaded new UI server configuration being output to the log before go test finishes, showing the tight loop it's entered).

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions