Skip to content

Commit

Permalink
Add Stop function to Config
Browse files Browse the repository at this point in the history
This prevents using it during or after the process teardown. Particulary relevant
to async work like the observers.
  • Loading branch information
EduardGomezEscandell committed Feb 9, 2024
1 parent 4eb8c24 commit f9f686c
Show file tree
Hide file tree
Showing 5 changed files with 92 additions and 9 deletions.
71 changes: 69 additions & 2 deletions windows-agent/internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ import (
// Config manages configuration parameters. It is a wrapper around a dictionary
// that reads and updates the config file.
type Config struct {
ctx context.Context
cancel func()

// data
subscription subscription
landscape landscapeConf
Expand All @@ -35,6 +38,7 @@ type Config struct {

// observers are called after any configuration changes.
observers []func()
wg sync.WaitGroup
}

// New creates and initializes a new Config object.
Expand All @@ -44,20 +48,59 @@ func New(ctx context.Context, cachePath string) (m *Config) {
mu: &sync.Mutex{},
}

m.ctx, m.cancel = context.WithCancel(ctx)

return m
}

// Stop releases all resources associated with the config.
func (c *Config) Stop() {
c.cancel()

c.mu.Lock()
defer c.mu.Unlock()

c.wg.Wait()
}

func (c *Config) stopped() bool {
select {
case <-c.ctx.Done():
return true
default:
return false
}
}

// Notify appends a callback. It'll be called every time any configuration changes.
func (c *Config) Notify(f func()) {
c.mu.Lock()
defer c.mu.Unlock()

c.observers = append(c.observers, f)
}

// notifyObservers calls all the observers. Use it under a mutex.
func (c *Config) notifyObservers() {
c.wg.Add(len(c.observers))

for _, f := range c.observers {
// This needs to be in a goroutine because notifyObservers is sometimes
f := f
// This needs to be in a goroutine because notifyObservers is always
// called under the config mutex. The callback trying to grab the mutex
// (to read the config) would cause a deadlock otherwise.
go f()
//
// We protect it under "stopped" to avoid running callbacks during the
// agent's shutdown.
go func() {
defer c.wg.Done()

if c.stopped() {
return
}

f()
}()
}
}

Expand All @@ -66,6 +109,10 @@ func (c *Config) Subscription(ctx context.Context) (token string, source Source,
c.mu.Lock()
defer c.mu.Unlock()

if c.stopped() {
return "", SourceNone, errors.New("config stopped")
}

if err := c.load(); err != nil {
return "", SourceNone, fmt.Errorf("could not load: %v", err)
}
Expand All @@ -82,6 +129,10 @@ func (c *Config) ProvisioningTasks(ctx context.Context, distroName string) ([]ta
c.mu.Lock()
defer c.mu.Unlock()

if c.stopped() {
return nil, errors.New("config stopped")
}

if err := c.load(); err != nil {
return nil, fmt.Errorf("could not load: %v", err)
}
Expand All @@ -103,6 +154,10 @@ func (c *Config) LandscapeClientConfig(ctx context.Context) (string, Source, err
c.mu.Lock()
defer c.mu.Unlock()

if c.stopped() {
return "", SourceNone, errors.New("config stopped")
}

if err := c.load(); err != nil {
return "", SourceNone, fmt.Errorf("could not load: %v", err)
}
Expand Down Expand Up @@ -139,6 +194,10 @@ func (c *Config) set(ctx context.Context, field *string, value string) error {
c.mu.Lock()
defer c.mu.Unlock()

if c.stopped() {
return errors.New("config stopped")
}

// Load before dumping to avoid overriding recent changes to file
if err := c.load(); err != nil {
return err
Expand All @@ -164,6 +223,10 @@ func (c *Config) LandscapeAgentUID(ctx context.Context) (string, error) {
c.mu.Lock()
defer c.mu.Unlock()

if c.stopped() {
return "", errors.New("config stopped")
}

if err := c.load(); err != nil {
return "", fmt.Errorf("could not load: %v", err)
}
Expand Down Expand Up @@ -246,6 +309,10 @@ func (c *Config) collectRegistrySettingsTasks(ctx context.Context, data Registry
c.mu.Lock()
defer c.mu.Unlock()

if c.stopped() {
return nil, errors.New("config stopped")
}

// Load up-to-date state
if err := c.load(); err != nil {
return nil, err
Expand Down
2 changes: 1 addition & 1 deletion windows-agent/internal/config/config_marshal.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func (c *Config) load() (err error) {
return nil
}

func (c Config) dump() (err error) {
func (c *Config) dump() (err error) {
defer decorate.OnError(&err, "could not store Config data")

h := marshalHelper{
Expand Down
8 changes: 8 additions & 0 deletions windows-agent/internal/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ func TestSubscription(t *testing.T) {

setup, dir := setUpMockSettings(t, ctx, db, tc.settingsState, tc.breakFile)
conf := config.New(ctx, dir)
defer conf.Stop()
setup(t, conf)

token, source, err := conf.Subscription(ctx)
Expand Down Expand Up @@ -146,6 +147,7 @@ func TestLandscapeConfig(t *testing.T) {

setup, dir := setUpMockSettings(t, ctx, db, tc.settingsState, tc.breakFile)
conf := config.New(ctx, dir)
defer conf.Stop()
setup(t, conf)

landscapeConf, source, err := conf.LandscapeClientConfig(ctx)
Expand Down Expand Up @@ -201,6 +203,7 @@ func TestLandscapeAgentUID(t *testing.T) {
}

conf := config.New(ctx, dir)
defer conf.Stop()
setup(t, conf)

v, err := conf.LandscapeAgentUID(ctx)
Expand Down Expand Up @@ -313,6 +316,7 @@ func TestSetUserSubscription(t *testing.T) {

setup, dir := setUpMockSettings(t, ctx, db, tc.settingsState, tc.breakFile)
conf := config.New(ctx, dir)
defer conf.Stop()
setup(t, conf)

token := "new_token"
Expand Down Expand Up @@ -370,6 +374,7 @@ func TestSetLandscapeAgentUID(t *testing.T) {

setup, dir := setUpMockSettings(t, ctx, db, tc.settingsState, tc.breakFile)
conf := config.New(ctx, dir)
defer conf.Stop()
setup(t, conf)

uid := "new_uid"
Expand Down Expand Up @@ -443,6 +448,7 @@ func TestFetchMicrosoftStoreSubscription(t *testing.T) {

setup, dir := setUpMockSettings(t, ctx, db, tc.settingsState, tc.breakConfigFile)
c := config.New(ctx, dir)
defer c.Stop()
setup(t, c)

// Set up the mock Microsoft store
Expand Down Expand Up @@ -548,6 +554,7 @@ func TestUpdateRegistryData(t *testing.T) {

_, dir := setUpMockSettings(t, ctx, db, tc.settingsState, tc.breakConfigFile)
c := config.New(ctx, dir)
defer c.Stop()

// Enter a first set of data to override the defaults
err = c.UpdateRegistryData(ctx, config.RegistryData{
Expand Down Expand Up @@ -651,6 +658,7 @@ func TestNotify(t *testing.T) {

_, dir := setUpMockSettings(t, ctx, db, untouched, false)
c := config.New(ctx, dir)
defer c.Stop()

var notifyCount atomic.Int32
var wantNotifyCount int32
Expand Down
17 changes: 11 additions & 6 deletions windows-agent/internal/proservices/proservices.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ type Manager struct {
landscapeService *landscape.Service
registryWatcher *registrywatcher.Service
db *database.DistroDB
conf *config.Config
}

// options are the configurable functional options for the daemon.
Expand Down Expand Up @@ -67,25 +68,25 @@ func New(ctx context.Context, publicDir, privateDir string, args ...Option) (s M
//[GitHub](https://github.com/canonical/ubuntu-pro-for-wsl/pull/438)
InitWSLAPI()

conf := config.New(ctx, privateDir)
s.conf = config.New(ctx, privateDir)

db, err := database.New(ctx, privateDir, conf)
db, err := database.New(ctx, privateDir, s.conf)
if err != nil {
return s, err
}
s.db = db

w := registrywatcher.New(ctx, conf, s.db, registrywatcher.WithRegistry(opts.registry))
w := registrywatcher.New(ctx, s.conf, s.db, registrywatcher.WithRegistry(opts.registry))
s.registryWatcher = &w
s.registryWatcher.Start()

if err := conf.FetchMicrosoftStoreSubscription(ctx); err != nil {
if err := s.conf.FetchMicrosoftStoreSubscription(ctx); err != nil {
log.Warningf(ctx, "%v", err)
}

s.uiService = ui.New(ctx, conf, s.db)
s.uiService = ui.New(ctx, s.conf, s.db)

landscape, err := landscape.New(ctx, conf, s.db)
landscape, err := landscape.New(ctx, s.conf, s.db)
if err != nil {
return s, err
}
Expand Down Expand Up @@ -117,6 +118,10 @@ func (m Manager) Stop(ctx context.Context) {
if m.db != nil {
m.db.Close(ctx)
}

if m.conf != nil {
m.conf.Stop()
}
}

// RegisterGRPCServices returns a new grpc Server with the 2 api services attached to it.
Expand Down
3 changes: 3 additions & 0 deletions windows-agent/internal/proservices/ui/ui_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ func TestNew(t *testing.T) {
defer db.Close(ctx)

conf := config.New(ctx, dir)
defer conf.Stop()

_ = ui.New(context.Background(), conf, db)
}
Expand Down Expand Up @@ -92,6 +93,8 @@ func TestAttachPro(t *testing.T) {
}

conf := config.New(ctx, dir)
defer conf.Stop()

if tc.higherPriorityToken {
err = conf.UpdateRegistryData(ctx, config.RegistryData{
UbuntuProToken: "organization_token",
Expand Down

0 comments on commit f9f686c

Please sign in to comment.