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

fix(registry): instantiate factory for each invocation #1614

Merged
merged 8 commits into from
Jun 5, 2024

Conversation

bassosimone
Copy link
Contributor

@bassosimone bassosimone commented Jun 5, 2024

The current structure of the *registry.Factory registry is that we register an experiment like this:

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:

// 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:

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:

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.

This diff refactors *engine.experiment to make the report field
goroutine safe. It also moves at the bottom of experiment.go code
that was intermixed with *engine.experiment methods.

Part of ooni/probe#2607
We originally developed asynchronous experiments as a mean to
support websteps, a nettest that returned more than one measurement
per invocation of its Measure method.

Since then, we removed websteps. Therefore, this code is currently
technically unused. Additionally, this code further complicates
implementing richer input, because it is another way of performing
measurements.

As such, in the interest of switfly moving forward with richer
input _and_ of simplifying the engine, we are now removing this
unused functionality from the tree.

Part of ooni/probe#2607
Conflicts:
	internal/engine/experiment.go
Conflicts:
	internal/engine/experiment.go
Copy link
Contributor

@DecFox DecFox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@bassosimone bassosimone merged commit 15dac36 into master Jun 5, 2024
19 checks passed
@bassosimone bassosimone deleted the issue/2607c branch June 5, 2024 18:02
@bassosimone bassosimone added the 2024-06-richer-input Tracking 2024-06 richer input work label Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2024-06-richer-input Tracking 2024-06 richer input work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants