Skip to content

Commit

Permalink
fix(registry): instantiate factory for each invocation (#1614)
Browse files Browse the repository at this point in the history
The current structure of the `*registry.Factory` registry is that we
register an experiment like this:

```Go
func init() {
	AllExperiments["dnscheck"] = &Factory{
		build: func(config interface{}) model.ExperimentMeasurer {
			return dnscheck.NewExperimentMeasurer(
				*config.(*dnscheck.Config),
			)
		},
		config:           &dnscheck.Config{},
		enabledByDefault: true,
		inputPolicy:      model.InputOrStaticDefault,
	}
}
```

Then, when we're setting options, we're modifying the `config` directly
with code like this:

```Go
// SetOptionAny sets an option given any value.
func (b *Factory) SetOptionAny(key string, value any) error {
	field, err := b.fieldbyname(b.config, key)
	if err != nil {
		return err
	}
	switch field.Kind() {
	case reflect.Int64:
		return b.setOptionInt(field, value)
	case reflect.Bool:
		return b.setOptionBool(field, value)
	case reflect.String:
		return b.setOptionString(field, value)
	default:
		return fmt.Errorf("%w: %T", ErrUnsupportedOptionType, value)
	}
}
```

Finally, we pass the modified config to a new experiment when we're
creating it:

```Go
func (b *Factory) NewExperimentMeasurer() model.ExperimentMeasurer {
	return b.build(b.config)
}
```

This means that, if we run two back experiments that both require
options, we're going to always reuse the same option structure. This
feels wrong. We should instead construct a new factory each time, so we
start from empty options:

```Go
func init() {
	AllExperiments["dnscheck"] = func() *Factory {
		return &Factory{
			build: func(config interface{}) model.ExperimentMeasurer {
				return dnscheck.NewExperimentMeasurer(
					*config.(*dnscheck.Config),
				)
			},
			config:           &dnscheck.Config{},
			enabledByDefault: true,
			inputPolicy:      model.InputOrStaticDefault,
		}
	}
}
```

This diff applies this very simple mechanical change.

Note that so far we were ~good with the current codebase because we
don't use options much and generally we invoke each experiment just once
per run. This is going to change with OONI Run v2 and richer input.
Therefore, it makes sense to tackle this issue now in the context of
ooni/probe#2607.
  • Loading branch information
bassosimone authored Jun 5, 2024
1 parent 885ce85 commit 15dac36
Show file tree
Hide file tree
Showing 34 changed files with 358 additions and 294 deletions.
3 changes: 2 additions & 1 deletion internal/cmd/miniooni/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ func registerOONIRun(rootCmd *cobra.Command, globalOptions *Options) {

// registerAllExperiments registers a subcommand for each experiment
func registerAllExperiments(rootCmd *cobra.Command, globalOptions *Options) {
for name, factory := range registry.AllExperiments {
for name, ff := range registry.AllExperiments {
subCmd := &cobra.Command{
Use: name,
Short: fmt.Sprintf("Runs the %s experiment", name),
Expand All @@ -243,6 +243,7 @@ func registerAllExperiments(rootCmd *cobra.Command, globalOptions *Options) {
}
rootCmd.AddCommand(subCmd)
flags := subCmd.Flags()
factory := ff()

switch factory.InputPolicy() {
case model.InputOrQueryBackend,
Expand Down
2 changes: 1 addition & 1 deletion internal/registry/allexperiments.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package registry
import "sort"

// Where we register all the available experiments.
var AllExperiments = map[string]*Factory{}
var AllExperiments = map[string]func() *Factory{}

// ExperimentNames returns the name of all experiments
func ExperimentNames() (names []string) {
Expand Down
22 changes: 12 additions & 10 deletions internal/registry/dash.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,17 @@ import (
)

func init() {
AllExperiments["dash"] = &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return dash.NewExperimentMeasurer(
*config.(*dash.Config),
)
},
config: &dash.Config{},
enabledByDefault: true,
interruptible: true,
inputPolicy: model.InputNone,
AllExperiments["dash"] = func() *Factory {
return &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return dash.NewExperimentMeasurer(
*config.(*dash.Config),
)
},
config: &dash.Config{},
enabledByDefault: true,
interruptible: true,
inputPolicy: model.InputNone,
}
}
}
20 changes: 11 additions & 9 deletions internal/registry/dnscheck.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,16 @@ import (
)

func init() {
AllExperiments["dnscheck"] = &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return dnscheck.NewExperimentMeasurer(
*config.(*dnscheck.Config),
)
},
config: &dnscheck.Config{},
enabledByDefault: true,
inputPolicy: model.InputOrStaticDefault,
AllExperiments["dnscheck"] = func() *Factory {
return &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return dnscheck.NewExperimentMeasurer(
*config.(*dnscheck.Config),
)
},
config: &dnscheck.Config{},
enabledByDefault: true,
inputPolicy: model.InputOrStaticDefault,
}
}
}
20 changes: 11 additions & 9 deletions internal/registry/dnsping.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,16 @@ import (
)

func init() {
AllExperiments["dnsping"] = &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return dnsping.NewExperimentMeasurer(
*config.(*dnsping.Config),
)
},
config: &dnsping.Config{},
enabledByDefault: true,
inputPolicy: model.InputOrStaticDefault,
AllExperiments["dnsping"] = func() *Factory {
return &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return dnsping.NewExperimentMeasurer(
*config.(*dnsping.Config),
)
},
config: &dnsping.Config{},
enabledByDefault: true,
inputPolicy: model.InputOrStaticDefault,
}
}
}
18 changes: 10 additions & 8 deletions internal/registry/dslxtutorial.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@ import (
)

func init() {
AllExperiments["simple_sni"] = &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return chapter02.NewExperimentMeasurer(
*config.(*chapter02.Config),
)
},
config: &chapter02.Config{},
inputPolicy: model.InputOrQueryBackend,
AllExperiments["simple_sni"] = func() *Factory {
return &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return chapter02.NewExperimentMeasurer(
*config.(*chapter02.Config),
)
},
config: &chapter02.Config{},
inputPolicy: model.InputOrQueryBackend,
}
}
}
18 changes: 10 additions & 8 deletions internal/registry/echcheck.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@ import (
)

func init() {
AllExperiments["echcheck"] = &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return echcheck.NewExperimentMeasurer(
*config.(*echcheck.Config),
)
},
config: &echcheck.Config{},
inputPolicy: model.InputOptional,
AllExperiments["echcheck"] = func() *Factory {
return &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return echcheck.NewExperimentMeasurer(
*config.(*echcheck.Config),
)
},
config: &echcheck.Config{},
inputPolicy: model.InputOptional,
}
}
}
28 changes: 15 additions & 13 deletions internal/registry/example.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,20 @@ import (
)

func init() {
AllExperiments["example"] = &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return example.NewExperimentMeasurer(
*config.(*example.Config), "example",
)
},
config: &example.Config{
Message: "Good day from the example experiment!",
SleepTime: int64(time.Second),
},
enabledByDefault: true,
interruptible: true,
inputPolicy: model.InputNone,
AllExperiments["example"] = func() *Factory {
return &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return example.NewExperimentMeasurer(
*config.(*example.Config), "example",
)
},
config: &example.Config{
Message: "Good day from the example experiment!",
SleepTime: int64(time.Second),
},
enabledByDefault: true,
interruptible: true,
inputPolicy: model.InputNone,
}
}
}
7 changes: 4 additions & 3 deletions internal/registry/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ func (b *Factory) fieldbyname(v interface{}, key string) (reflect.Value, error)
return field, nil
}

// NewExperimentMeasurer creates the experiment
// NewExperimentMeasurer creates a new [model.ExperimentMeasurer] instance.
func (b *Factory) NewExperimentMeasurer() model.ExperimentMeasurer {
return b.build(b.config)
}
Expand Down Expand Up @@ -305,10 +305,11 @@ func NewFactory(name string, kvStore model.KeyValueStore, logger model.Logger) (
}

// Obtain the factory for the canonical name.
factory := AllExperiments[name]
if factory == nil {
ff := AllExperiments[name]
if ff == nil {
return nil, fmt.Errorf("%w: %s", ErrNoSuchExperiment, name)
}
factory := ff()

// Some experiments are not enabled by default. To enable them we use
// the cached check-in response or an environment variable.
Expand Down
20 changes: 11 additions & 9 deletions internal/registry/fbmessenger.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,16 @@ import (
)

func init() {
AllExperiments["facebook_messenger"] = &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return fbmessenger.NewExperimentMeasurer(
*config.(*fbmessenger.Config),
)
},
config: &fbmessenger.Config{},
enabledByDefault: true,
inputPolicy: model.InputNone,
AllExperiments["facebook_messenger"] = func() *Factory {
return &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return fbmessenger.NewExperimentMeasurer(
*config.(*fbmessenger.Config),
)
},
config: &fbmessenger.Config{},
enabledByDefault: true,
inputPolicy: model.InputNone,
}
}
}
20 changes: 11 additions & 9 deletions internal/registry/hhfm.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,16 @@ import (
)

func init() {
AllExperiments["http_header_field_manipulation"] = &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return hhfm.NewExperimentMeasurer(
*config.(*hhfm.Config),
)
},
config: &hhfm.Config{},
enabledByDefault: true,
inputPolicy: model.InputNone,
AllExperiments["http_header_field_manipulation"] = func() *Factory {
return &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return hhfm.NewExperimentMeasurer(
*config.(*hhfm.Config),
)
},
config: &hhfm.Config{},
enabledByDefault: true,
inputPolicy: model.InputNone,
}
}
}
20 changes: 11 additions & 9 deletions internal/registry/hirl.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,16 @@ import (
)

func init() {
AllExperiments["http_invalid_request_line"] = &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return hirl.NewExperimentMeasurer(
*config.(*hirl.Config),
)
},
config: &hirl.Config{},
enabledByDefault: true,
inputPolicy: model.InputNone,
AllExperiments["http_invalid_request_line"] = func() *Factory {
return &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return hirl.NewExperimentMeasurer(
*config.(*hirl.Config),
)
},
config: &hirl.Config{},
enabledByDefault: true,
inputPolicy: model.InputNone,
}
}
}
20 changes: 11 additions & 9 deletions internal/registry/httphostheader.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,16 @@ import (
)

func init() {
AllExperiments["http_host_header"] = &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return httphostheader.NewExperimentMeasurer(
*config.(*httphostheader.Config),
)
},
config: &httphostheader.Config{},
enabledByDefault: true,
inputPolicy: model.InputOrQueryBackend,
AllExperiments["http_host_header"] = func() *Factory {
return &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return httphostheader.NewExperimentMeasurer(
*config.(*httphostheader.Config),
)
},
config: &httphostheader.Config{},
enabledByDefault: true,
inputPolicy: model.InputOrQueryBackend,
}
}
}
22 changes: 12 additions & 10 deletions internal/registry/ndt.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,17 @@ import (
)

func init() {
AllExperiments["ndt"] = &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return ndt7.NewExperimentMeasurer(
*config.(*ndt7.Config),
)
},
config: &ndt7.Config{},
enabledByDefault: true,
interruptible: true,
inputPolicy: model.InputNone,
AllExperiments["ndt"] = func() *Factory {
return &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return ndt7.NewExperimentMeasurer(
*config.(*ndt7.Config),
)
},
config: &ndt7.Config{},
enabledByDefault: true,
interruptible: true,
inputPolicy: model.InputNone,
}
}
}
22 changes: 12 additions & 10 deletions internal/registry/portfiltering.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,17 @@ import (
)

func init() {
AllExperiments["portfiltering"] = &Factory{
build: func(config any) model.ExperimentMeasurer {
return portfiltering.NewExperimentMeasurer(
config.(portfiltering.Config),
)
},
config: portfiltering.Config{},
enabledByDefault: true,
interruptible: false,
inputPolicy: model.InputNone,
AllExperiments["portfiltering"] = func() *Factory {
return &Factory{
build: func(config any) model.ExperimentMeasurer {
return portfiltering.NewExperimentMeasurer(
config.(portfiltering.Config),
)
},
config: portfiltering.Config{},
enabledByDefault: true,
interruptible: false,
inputPolicy: model.InputNone,
}
}
}
20 changes: 11 additions & 9 deletions internal/registry/psiphon.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,16 @@ import (
)

func init() {
AllExperiments["psiphon"] = &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return psiphon.NewExperimentMeasurer(
*config.(*psiphon.Config),
)
},
config: &psiphon.Config{},
enabledByDefault: true,
inputPolicy: model.InputOptional,
AllExperiments["psiphon"] = func() *Factory {
return &Factory{
build: func(config interface{}) model.ExperimentMeasurer {
return psiphon.NewExperimentMeasurer(
*config.(*psiphon.Config),
)
},
config: &psiphon.Config{},
enabledByDefault: true,
inputPolicy: model.InputOptional,
}
}
}
Loading

0 comments on commit 15dac36

Please sign in to comment.