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

Race condition on live configuration update #44

Closed
oklahomer opened this issue Jul 8, 2017 · 1 comment
Closed

Race condition on live configuration update #44

oklahomer opened this issue Jul 8, 2017 · 1 comment

Comments

@oklahomer
Copy link
Owner

Sarah provides a mechanism to update configuration struct for both Command and ScheduledTask when corresponding configuration file is updated. Race condition may occur in below situations:

  1. Configuration file content is mapped to existing struct's instance.
  2. Re-built Command or ScheduledTask replaces existing one.

The first one occurs because updated configuration values are set to existing config instance; Config struct is not cloned or instantiated every time live update runs. This is by design. CommandPropsBuilder.ConfigurableFunc and ScheduledTaskPropsBuilder.ConfigurableFunc are designed to receive config instance instead of reflect.Type so a struct instance with non-zero value field can be passed. e.g. 1) NewConfig returns config with pre-set default value and other fields are read from configuration file on Command (re-)build. 2) A DB connection is set to config struct beforehand and is expected to stay while other fields with json/yaml tags are updated on live-update. So cloning or instantiating config struct on live update is not an option in this case. In addition, cloning or deep copying can be complicated when field value is IO related object such as file handle. A locking mechanism seems appropriate.

The Second case is rather a typical read/write collision. Commands type is merely an extension of slice, and therefore locking mechanism is required when replacing its element.

@oklahomer
Copy link
Owner Author

oklahomer commented Jul 8, 2017

The easiest way to reproduce both cases is as below:

func Test_race(t *testing.T) {
        // Prepare CommandProps
	type config struct {
		Token string
	}
	props, err := NewCommandPropsBuilder().
		Identifier("dummy").
		InputExample(".dummy").
		BotType("dummyBot").
		ConfigurableFunc(
			&config{Token: "default"},
			func(ctx context.Context, _ Input, givenConfig CommandConfig) (*CommandResponse, error) {
				fmt.Print(givenConfig.(*config).Token) // Read
				return nil, nil
			},
		).
		MatchFunc(func(_ Input) bool { return true }).
		Build()
	if err != nil {
		t.Fatalf("Error on CommnadProps preparation: %s.", err.Error())
	}

        // Prepare a bot
	commands := NewCommands()
	bot := &DummyBot{
		RespondFunc: func(ctx context.Context, input Input) error {
			_, err := commands.ExecuteFirstMatched(ctx, input)
			return err
		},
		AppendCommandFunc: func(cmd Command) {
			commands.Append(cmd)
		},
	}

	rootCtx := context.Background()
	ctx, cancel := context.WithCancel(rootCtx)

        // Continuously read configuration file and re-build Command
	go func(c context.Context, b Bot, p *CommandProps) {
		for {
			select {
			case <-c.Done():
				return

			default:
				// Write
				command, err := newCommand(p, filepath.Join("testdata", "command"))
				if err == nil {
					b.AppendCommand(command)
				} else {
					t.Errorf("Error on command build: %s.", err.Error())
				}

			}
		}
	}(ctx, bot, props)

        // Continuously read config struct's field value by calling Bot.Respond
	go func(c context.Context, b Bot) {
		for {
			select {
			case <-c.Done():
				return

			default:
				b.Respond(c, &DummyInput{})

			}
		}
	}(ctx, bot)

	// Wait till race condition occurs
	time.Sleep(1 * time.Second)
	cancel()
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant