fix: ensure experiments return nil when we want to submit (#654)

Since https://github.com/ooni/probe-cli/pull/527, if an experiment
returns an error, the corresponding measurement is not submitted since
the semantics of returning an error is that something fundamental
went wrong (e.g., we could not parse the input URL).

This diff ensures that all experiments only return and error when
something fundamental was wrong and return nil otherwise.

Reference issue: https://github.com/ooni/probe/issues/1808.
This commit is contained in:
Simone Basso 2022-01-07 13:17:20 +01:00 committed by GitHub
parent 60a3c372f5
commit 99ec7ffca9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
27 changed files with 132 additions and 66 deletions

View File

@ -25,7 +25,7 @@ const (
defaultTimeout = 120 * time.Second
magicVersion = "0.008000000"
testName = "dash"
testVersion = "0.12.0"
testVersion = "0.13.0"
totalStep = 15.0
)
@ -273,7 +273,13 @@ func (m Measurer) Run(
}
ctx, cancel := context.WithTimeout(ctx, defaultTimeout)
defer cancel()
return r.do(ctx)
// Implementation note: we ignore the return value of r.do rather than
// returning it to the caller. We do that because returning an error means
// the measurement failed for some fundamental reason (e.g., the input
// is an URL that you cannot parse). For DASH, this case will never happen
// because there is no input, so always returning nil is fine here.
_ = r.do(ctx)
return nil
}
// NewExperimentMeasurer creates a new ExperimentMeasurer.

View File

@ -261,7 +261,7 @@ func TestNewExperimentMeasurer(t *testing.T) {
if measurer.ExperimentName() != "dash" {
t.Fatal("unexpected name")
}
if measurer.ExperimentVersion() != "0.12.0" {
if measurer.ExperimentVersion() != "0.13.0" {
t.Fatal("unexpected version")
}
}
@ -280,7 +280,9 @@ func TestMeasureWithCancelledContext(t *testing.T) {
measurement,
model.NewPrinterCallbacks(log.Log),
)
if !errors.Is(err, context.Canceled) {
// See corresponding comment in Measurer.Run implementation to
// understand why here it's correct to return nil.
if !errors.Is(err, nil) {
t.Fatal("unexpected error value")
}
sk, err := m.GetSummaryKeys(measurement)

View File

@ -0,0 +1,4 @@
// Package dash implements the DASH network experiment.
//
// Spec: https://github.com/ooni/spec/blob/master/nettests/ts-021-dash.md
package dash

View File

@ -159,6 +159,9 @@ func (m *Measurer) Run(
return ErrUnsupportedURLScheme
}
// Implementation note: we must not return an error from now now. Returning an
// error means that we don't have a measurement to submit.
// 4. possibly expand a domain to a list of IP addresses.
//
// Implementation note: because the resolver we constructed also deals

View File

@ -73,6 +73,10 @@ func (m Measurer) Run(
<-ctx.Done()
sess.Logger().Infof("%s", "Knock, knock, Neo.")
callbacks.OnProgress(1.0, m.config.Message)
// Note: if here we return an error, the parent code will assume
// something fundamental was wrong and we don't have a measurement
// to submit to the OONI collector. Keep this in mind when you
// are writing new experiments!
return err
}

View File

@ -55,6 +55,9 @@ func (mgr dialManager) dialWithTestName(ctx context.Context, testName string) (*
headers.Add("User-Agent", mgr.userAgent)
mgr.logrequest(mgr.ndt7URL, headers)
conn, _, err := dialer.DialContext(ctx, mgr.ndt7URL, headers)
if err != nil {
err = netxlite.NewTopLevelGenericErrWrapper(err)
}
mgr.logresponse(err)
return conn, err
}

View File

@ -6,11 +6,11 @@ import (
"net/http"
"net/http/httptest"
"net/url"
"strings"
"testing"
"github.com/apex/log"
"github.com/gorilla/websocket"
"github.com/ooni/probe-cli/v3/internal/netxlite"
)
func TestDialDownloadWithCancelledContext(t *testing.T) {
@ -18,7 +18,7 @@ func TestDialDownloadWithCancelledContext(t *testing.T) {
cancel() // immediately halt
mgr := newDialManager("wss://hostname.fake", log.Log, "miniooni/0.1.0-dev")
conn, err := mgr.dialDownload(ctx)
if err == nil || !strings.HasSuffix(err.Error(), "context canceled") {
if err == nil || err.Error() != netxlite.FailureInterrupted {
t.Fatal("not the error we expected", err)
}
if conn != nil {
@ -31,7 +31,7 @@ func TestDialUploadWithCancelledContext(t *testing.T) {
cancel() // immediately halt
mgr := newDialManager("wss://hostname.fake", log.Log, "miniooni/0.1.0-dev")
conn, err := mgr.dialUpload(ctx)
if err == nil || !strings.HasSuffix(err.Error(), "context canceled") {
if err == nil || err.Error() != netxlite.FailureInterrupted {
t.Fatal("not the error we expected", err)
}
if conn != nil {

View File

@ -0,0 +1,4 @@
// Package ndt7 contains the ndt7 network experiment.
//
// See https://github.com/ooni/spec/blob/master/nettests/ts-022-ndt.md
package ndt7

View File

@ -1,6 +1,3 @@
// Package ndt7 contains the ndt7 network experiment.
//
// See https://github.com/ooni/spec/blob/master/nettests/ts-022-ndt.md
package ndt7
import (
@ -8,18 +5,17 @@ import (
"encoding/json"
"errors"
"fmt"
"net/http"
"time"
"github.com/ooni/probe-cli/v3/internal/engine/netx"
"github.com/ooni/probe-cli/v3/internal/humanize"
"github.com/ooni/probe-cli/v3/internal/mlablocatev2"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/netxlite"
)
const (
testName = "ndt"
testVersion = "0.9.0"
testVersion = "0.10.0"
)
// Config contains the experiment settings
@ -80,11 +76,7 @@ type Measurer struct {
func (m *Measurer) discover(
ctx context.Context, sess model.ExperimentSession) (mlablocatev2.NDT7Result, error) {
httpClient := &http.Client{
Transport: netx.NewHTTPTransport(netx.Config{
Logger: sess.Logger(),
}),
}
httpClient := netxlite.NewHTTPClientStdlib(sess.Logger())
defer httpClient.CloseIdleConnections()
client := mlablocatev2.NewClient(httpClient, sess.Logger(), sess.UserAgent())
out, err := client.QueryNDT7(ctx)
@ -228,7 +220,7 @@ func (m *Measurer) Run(
locateResult, err := m.discover(ctx, sess)
if err != nil {
tk.Failure = failureFromError(err)
return err
return nil // we still want to submit this measurement
}
tk.Server = ServerInfo{
Hostname: locateResult.Hostname,
@ -240,7 +232,7 @@ func (m *Measurer) Run(
}
if err := m.doDownload(ctx, sess, callbacks, tk, locateResult.WSSDownloadURL); err != nil {
tk.Failure = failureFromError(err)
return err
return nil // we still want to submit this measurement
}
callbacks.OnProgress(0.5, fmt.Sprintf(" upload: url: %s", locateResult.WSSUploadURL))
if m.preUploadHook != nil {
@ -248,7 +240,7 @@ func (m *Measurer) Run(
}
if err := m.doUpload(ctx, sess, callbacks, tk, locateResult.WSSUploadURL); err != nil {
tk.Failure = failureFromError(err)
return err
return nil // we still want to submit this measurement
}
return nil
}

View File

@ -4,12 +4,12 @@ import (
"context"
"errors"
"net/http"
"strings"
"testing"
"github.com/apex/log"
"github.com/ooni/probe-cli/v3/internal/engine/mockable"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/netxlite"
)
func TestNewExperimentMeasurer(t *testing.T) {
@ -17,7 +17,7 @@ func TestNewExperimentMeasurer(t *testing.T) {
if measurer.ExperimentName() != "ndt" {
t.Fatal("unexpected name")
}
if measurer.ExperimentVersion() != "0.9.0" {
if measurer.ExperimentVersion() != "0.10.0" {
t.Fatal("unexpected version")
}
}
@ -52,7 +52,7 @@ func TestDoDownloadWithCancelledContext(t *testing.T) {
err := m.doDownload(
ctx, sess, model.NewPrinterCallbacks(log.Log), new(TestKeys),
"ws://host.name")
if err == nil || !strings.HasSuffix(err.Error(), "context canceled") {
if err == nil || err.Error() != netxlite.FailureInterrupted {
t.Fatal("not the error we expected", err)
}
}
@ -69,7 +69,7 @@ func TestDoUploadWithCancelledContext(t *testing.T) {
err := m.doUpload(
ctx, sess, model.NewPrinterCallbacks(log.Log), new(TestKeys),
"ws://host.name")
if err == nil || !strings.HasSuffix(err.Error(), "context canceled") {
if err == nil || err.Error() != netxlite.FailureInterrupted {
t.Fatal("not the error we expected", err)
}
}
@ -83,10 +83,19 @@ func TestRunWithCancelledContext(t *testing.T) {
}
ctx, cancel := context.WithCancel(context.Background())
cancel() // immediately cancel
err := m.Run(ctx, sess, new(model.Measurement), model.NewPrinterCallbacks(log.Log))
if !errors.Is(err, context.Canceled) {
meas := &model.Measurement{}
err := m.Run(ctx, sess, meas, model.NewPrinterCallbacks(log.Log))
// Here we get nil because we still want to submit this measurement
if !errors.Is(err, nil) {
t.Fatal("not the error we expected")
}
if meas.TestKeys == nil {
t.Fatal("nil test keys")
}
tk := meas.TestKeys.(*TestKeys)
if tk.Failure == nil || *tk.Failure != netxlite.FailureInterrupted {
t.Fatal("unexpected tk.Failure")
}
}
func TestGood(t *testing.T) {
@ -123,17 +132,27 @@ func TestFailDownload(t *testing.T) {
measurer.preDownloadHook = func() {
cancel()
}
meas := &model.Measurement{}
err := measurer.Run(
ctx,
&mockable.Session{
MockableHTTPClient: http.DefaultClient,
MockableLogger: log.Log,
},
new(model.Measurement),
meas,
model.NewPrinterCallbacks(log.Log),
)
if err == nil || !strings.HasSuffix(err.Error(), "context canceled") {
t.Fatal("not the error we expected", err)
// We expect a nil failure here because we want to submit anyway
// a measurement that failed to connect to m-lab.
if err != nil {
t.Fatal(err)
}
if meas.TestKeys == nil {
t.Fatal("expected non-nil TestKeys here")
}
tk := meas.TestKeys.(*TestKeys)
if tk.Failure == nil || *tk.Failure != netxlite.FailureInterrupted {
t.Fatal("unexpected tk.Failure")
}
}
@ -144,17 +163,26 @@ func TestFailUpload(t *testing.T) {
measurer.preUploadHook = func() {
cancel()
}
meas := &model.Measurement{}
err := measurer.Run(
ctx,
&mockable.Session{
MockableHTTPClient: http.DefaultClient,
MockableLogger: log.Log,
},
new(model.Measurement),
meas,
model.NewPrinterCallbacks(log.Log),
)
if err == nil || !strings.HasSuffix(err.Error(), "context canceled") {
t.Fatal("not the error we expected", err)
// Here we expect a nil error because we want to submit this measurement
if err != nil {
t.Fatal(err)
}
if meas.TestKeys == nil {
t.Fatal("expected non-nil tk.TestKeys here")
}
tk := meas.TestKeys.(*TestKeys)
if tk.Failure == nil || *tk.Failure != netxlite.FailureInterrupted {
t.Fatal("unexpected tk.Failure value")
}
}

View File

@ -16,7 +16,7 @@ import (
const (
testName = "psiphon"
testVersion = "0.5.1"
testVersion = "0.6.0"
)
// Config contains the experiment's configuration.
@ -92,14 +92,14 @@ func (m *Measurer) Run(
if m.BeforeGetHook != nil {
m.BeforeGetHook(g)
}
tk, err := g.Get(ctx)
tk, _ := g.Get(ctx) // ignore error since we have the testkeys and want to submit them
cancel()
wg.Wait()
measurement.TestKeys = &TestKeys{
TestKeys: tk,
MaxRuntime: maxruntime,
}
return err
return nil
}
// NewExperimentMeasurer creates a new ExperimentMeasurer.

View File

@ -23,7 +23,7 @@ func TestNewExperimentMeasurer(t *testing.T) {
if measurer.ExperimentName() != "psiphon" {
t.Fatal("unexpected name")
}
if measurer.ExperimentVersion() != "0.5.1" {
if measurer.ExperimentVersion() != "0.6.0" {
t.Fatal("unexpected version")
}
}
@ -35,7 +35,7 @@ func TestRunWithCancelledContext(t *testing.T) {
measurement := new(model.Measurement)
err := measurer.Run(ctx, newfakesession(), measurement,
model.NewPrinterCallbacks(log.Log))
if !errors.Is(err, context.Canceled) {
if !errors.Is(err, nil) { // nil because we want to submit the measurement
t.Fatal("expected another error here")
}
tk := measurement.TestKeys.(*psiphon.TestKeys)
@ -66,7 +66,7 @@ func TestRunWithCustomInputAndCancelledContext(t *testing.T) {
cancel() // fail immediately
err := measurer.Run(ctx, newfakesession(), measurement,
model.NewPrinterCallbacks(log.Log))
if !errors.Is(err, context.Canceled) {
if !errors.Is(err, nil) { // nil because we want to submit the measurement
t.Fatal("expected another error here")
}
tk := measurement.TestKeys.(*psiphon.TestKeys)
@ -85,7 +85,7 @@ func TestRunWillPrintSomethingWithCancelledContext(t *testing.T) {
}
observer := observerCallbacks{progress: &atomicx.Int64{}}
err := measurer.Run(ctx, newfakesession(), measurement, observer)
if !errors.Is(err, context.Canceled) {
if !errors.Is(err, nil) { // nil because we want to submit the measurement
t.Fatal("expected another error here")
}
tk := measurement.TestKeys.(*psiphon.TestKeys)

View File

@ -2,7 +2,6 @@ package run
import (
"context"
"sync"
"github.com/ooni/probe-cli/v3/internal/engine/experiment/dnscheck"
"github.com/ooni/probe-cli/v3/internal/model"
@ -10,7 +9,6 @@ import (
type dnsCheckMain struct {
Endpoints *dnscheck.Endpoints
mu sync.Mutex
}
func (m *dnsCheckMain) do(ctx context.Context, input StructuredInput,

View File

@ -0,0 +1,4 @@
// Package run contains code to run other experiments.
//
// This code is currently alpha.
package run

View File

@ -1,6 +1,3 @@
// Package run contains code to run other experiments.
//
// This code is currently alpha.
package run
import (

View File

@ -63,7 +63,7 @@ func TestRunURLGetterWithCancelledContext(t *testing.T) {
sess := &mockable.Session{MockableLogger: log.Log}
callbacks := model.NewPrinterCallbacks(log.Log)
err := measurer.Run(ctx, sess, measurement, callbacks)
if err == nil {
if err != nil { // here we expected nil b/c we want to submit the measurement
t.Fatal(err)
}
if len(measurement.Extensions) != 6 {

View File

@ -22,7 +22,7 @@ import (
const (
testName = "stunreachability"
testVersion = "0.3.0"
testVersion = "0.4.0"
)
// Config contains the experiment config.
@ -100,7 +100,7 @@ func (m *Measurer) Run(
if err := wrap(tk.run(ctx, m.config, sess, measurement, callbacks, URL.Host)); err != nil {
s := err.Error()
tk.Failure = &s
return err
return nil // we want to submit this measurement
}
return nil
}

View File

@ -24,7 +24,7 @@ func TestMeasurerExperimentNameVersion(t *testing.T) {
if measurer.ExperimentName() != "stunreachability" {
t.Fatal("unexpected ExperimentName")
}
if measurer.ExperimentVersion() != "0.3.0" {
if measurer.ExperimentVersion() != "0.4.0" {
t.Fatal("unexpected ExperimentVersion")
}
}
@ -128,7 +128,7 @@ func TestCancelledContext(t *testing.T) {
measurement,
model.NewPrinterCallbacks(log.Log),
)
if !errors.Is(err, context.Canceled) {
if !errors.Is(err, nil) { // nil because we want to submit
t.Fatal("not the error we expected", err)
}
tk := measurement.TestKeys.(*TestKeys)
@ -168,7 +168,7 @@ func TestNewClientFailure(t *testing.T) {
measurement,
model.NewPrinterCallbacks(log.Log),
)
if !errors.Is(err, expected) {
if !errors.Is(err, nil) { // nil because we want to submit
t.Fatal("not the error we expected")
}
tk := measurement.TestKeys.(*TestKeys)
@ -202,7 +202,7 @@ func TestStartFailure(t *testing.T) {
measurement,
model.NewPrinterCallbacks(log.Log),
)
if !errors.Is(err, expected) {
if !errors.Is(err, nil) { // nil because we want to submit
t.Fatal("not the error we expected")
}
tk := measurement.TestKeys.(*TestKeys)
@ -240,7 +240,7 @@ func TestReadFailure(t *testing.T) {
measurement,
model.NewPrinterCallbacks(log.Log),
)
if !errors.Is(err, stun.ErrTransactionTimeOut) {
if !errors.Is(err, nil) { // nil because we want to submit
t.Fatal("not the error we expected")
}
tk := measurement.TestKeys.(*TestKeys)

View File

@ -104,7 +104,7 @@ func (m Measurer) Run(
Failure: archival.NewFailure(err),
}
}
return nil
return nil // return nil so we always submit the measurement
}
func (m Measurer) newDialer(logger model.Logger) netx.Dialer {

View File

@ -174,7 +174,7 @@ func (m *Measurer) Run(
) error {
targets, err := m.gimmeTargets(ctx, sess)
if err != nil {
return err
return err // fail the measurement if we cannot get any target
}
registerExtensions(measurement)
m.measureTargets(ctx, sess, measurement, callbacks, targets)

View File

@ -14,7 +14,7 @@ import (
const (
testName = "urlgetter"
testVersion = "0.1.0"
testVersion = "0.2.0"
)
// Config contains the experiment's configuration.
@ -109,9 +109,9 @@ func (m Measurer) Run(
Session: sess,
Target: string(measurement.Input),
}
tk, err := g.Get(ctx)
tk, _ := g.Get(ctx) // ignore error since we have the testkeys and we wanna submit them
measurement.TestKeys = &tk
return err
return nil
}
// NewExperimentMeasurer creates a new ExperimentMeasurer.

View File

@ -18,7 +18,7 @@ func TestMeasurer(t *testing.T) {
if m.ExperimentName() != "urlgetter" {
t.Fatal("invalid experiment name")
}
if m.ExperimentVersion() != "0.1.0" {
if m.ExperimentVersion() != "0.2.0" {
t.Fatal("invalid experiment version")
}
measurement := new(model.Measurement)
@ -27,7 +27,7 @@ func TestMeasurer(t *testing.T) {
ctx, &mockable.Session{},
measurement, model.NewPrinterCallbacks(log.Log),
)
if !errors.Is(err, context.Canceled) {
if !errors.Is(err, nil) { // nil because we want to submit the measurement
t.Fatal("not the error we expected")
}
if len(measurement.Extensions) != 6 {
@ -55,7 +55,7 @@ func TestMeasurerDNSCache(t *testing.T) {
if m.ExperimentName() != "urlgetter" {
t.Fatal("invalid experiment name")
}
if m.ExperimentVersion() != "0.1.0" {
if m.ExperimentVersion() != "0.2.0" {
t.Fatal("invalid experiment version")
}
measurement := new(model.Measurement)
@ -64,7 +64,7 @@ func TestMeasurerDNSCache(t *testing.T) {
ctx, &mockable.Session{},
measurement, model.NewPrinterCallbacks(log.Log),
)
if !errors.Is(err, context.Canceled) {
if !errors.Is(err, nil) { // nil because we want to submit the measurement
t.Fatal("not the error we expected")
}
if len(measurement.Extensions) != 6 {

View File

@ -0,0 +1,4 @@
// Package webconnectivity implements OONI's Web Connectivity experiment.
//
// See https://github.com/ooni/spec/blob/master/nettests/ts-017-web-connectivity.md
package webconnectivity

View File

@ -1,6 +1,3 @@
// Package webconnectivity implements OONI's Web Connectivity experiment.
//
// See https://github.com/ooni/spec/blob/master/nettests/ts-017-web-connectivity.md
package webconnectivity
import (

View File

@ -105,7 +105,7 @@ type ExperimentMeasurer interface {
// return an error in case the experiment could not run (e.g.,
// a required input is missing). Otherwise, the code should just
// set the relevant OONI error inside of the measurement and
// return nil. This is important because the caller may not submit
// return nil. This is important because the caller WILL NOT submit
// the measurement if this method returns an error.
Run(
ctx context.Context, sess ExperimentSession,

View File

@ -257,6 +257,13 @@ func NewHTTPTransportStdlib(logger model.DebugLogger) model.HTTPTransport {
return NewHTTPTransport(logger, dialer, tlsDialer)
}
// NewHTTPClientStdlib creates a new HTTPClient that uses the
// standard library for TLS and DNS resolutions.
func NewHTTPClientStdlib(logger model.DebugLogger) model.HTTPClient {
txp := NewHTTPTransportStdlib(logger)
return WrapHTTPClient(&http.Client{Transport: txp})
}
// WrapHTTPClient wraps an HTTP client to add error wrapping capabilities.
func WrapHTTPClient(clnt model.HTTPClient) model.HTTPClient {
return &httpClientErrWrapper{clnt}

View File

@ -13,6 +13,7 @@ import (
"github.com/apex/log"
oohttp "github.com/ooni/oohttp"
"github.com/ooni/probe-cli/v3/internal/atomicx"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/model/mocks"
)
@ -500,6 +501,18 @@ func TestHTTPClientErrWrapper(t *testing.T) {
})
}
func TestNewHTTPClientStdlib(t *testing.T) {
clnt := NewHTTPClientStdlib(model.DiscardLogger)
ewc, ok := clnt.(*httpClientErrWrapper)
if !ok {
t.Fatal("expected *httpClientErrWrapper")
}
_, ok = ewc.HTTPClient.(*http.Client)
if !ok {
t.Fatal("expected *http.Client")
}
}
func TestWrapHTTPClient(t *testing.T) {
origClient := &http.Client{}
wrapped := WrapHTTPClient(origClient)