refactor(netx): the TimeoutDialer is useless (#366)

We already configure a timeout in the underlying dialer, hence
there's no point in keeping the TimeoutDialer around.

Part of https://github.com/ooni/probe/issues/1507

internal/engine/experiment/ndt7/dial.go

@@ -36,7 +36,6 @@ func (mgr dialManager) dialWithTestName(ctx context.Context, testName string) (*
 	var reso resolver.Resolver = resolver.SystemResolver{}
 	reso = resolver.LoggingResolver{Resolver: reso, Logger: mgr.logger}
 	var dlr dialer.Dialer = dialer.Default
-	dlr = dialer.TimeoutDialer{Dialer: dlr}
 	dlr = dialer.ErrorWrapperDialer{Dialer: dlr}
 	dlr = dialer.LoggingDialer{Dialer: dlr, Logger: mgr.logger}
 	dlr = dialer.DNSDialer{Dialer: dlr, Resolver: reso}

internal/engine/legacy/netx/dialer.go

@@ -60,19 +60,16 @@ func maybeWithMeasurementRoot(
 // - DNSDialer (topmost)
 // - EmitterDialer
 // - ErrorWrapperDialer
-// - TimeoutDialer
 // - ByteCountingDialer
-// - net.Dialer
+// - dialer.Default
 //
 // If you have others needs, manually build the chain you need.
 func newDNSDialer(resolver dialer.Resolver) dialer.DNSDialer {
 	return dialer.DNSDialer{
 		Dialer: dialer.EmitterDialer{
 			Dialer: dialer.ErrorWrapperDialer{
-				Dialer: dialer.TimeoutDialer{
-					Dialer: dialer.ByteCounterDialer{
-						Dialer: new(net.Dialer),
-					},
+				Dialer: dialer.ByteCounterDialer{
+					Dialer: dialer.Default,
 				},
 			},
 		},

internal/engine/netx/dialer/system.go

@@ -6,8 +6,8 @@ import (
 	"time"
 )
 
-// defaultNetDialer is the net.Dialer we use by default.
-var defaultNetDialer = &net.Dialer{
+// underlyingDialer is the underlying net.Dialer.
+var underlyingDialer = &net.Dialer{
 	Timeout:   15 * time.Second,
 	KeepAlive: 15 * time.Second,
 }
@@ -17,7 +17,7 @@ type SystemDialer struct{}
 
 // DialContext implements Dialer.DialContext
 func (d SystemDialer) DialContext(ctx context.Context, network, address string) (net.Conn, error) {
-	return defaultNetDialer.DialContext(ctx, network, address)
+	return underlyingDialer.DialContext(ctx, network, address)
 }
 
 // Default is the dialer we use by default.

internal/engine/netx/dialer/system_test.go

@@ -3,11 +3,12 @@ package dialer
 import (
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/ooni/psiphon/oopsi/golang.org/x/net/context"
 )
 
-func TestSystemDialer(t *testing.T) {
+func TestSystemDialerWorks(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	cancel() // fail immediately
 	conn, err := Default.DialContext(ctx, "tcp", "8.8.8.8:853")
@@ -18,3 +19,10 @@ func TestSystemDialer(t *testing.T) {
 		t.Fatal("expected nil conn here")
 	}
 }
+
+func TestUnderlyingDialerHasTimeout(t *testing.T) {
+	expected := 15 * time.Second
+	if underlyingDialer.Timeout != expected {
+		t.Fatal("unexpected timeout value")
+	}
+}

internal/engine/netx/dialer/timeout.go

@@ -1,24 +0,0 @@
-package dialer
-
-import (
-	"context"
-	"net"
-	"time"
-)
-
-// TimeoutDialer is a Dialer that enforces a timeout
-type TimeoutDialer struct {
-	Dialer
-	ConnectTimeout time.Duration // default: 30 seconds
-}
-
-// DialContext implements Dialer.DialContext
-func (d TimeoutDialer) DialContext(ctx context.Context, network, address string) (net.Conn, error) {
-	timeout := 30 * time.Second
-	if d.ConnectTimeout != 0 {
-		timeout = d.ConnectTimeout
-	}
-	ctx, cancel := context.WithTimeout(ctx, timeout)
-	defer cancel()
-	return d.Dialer.DialContext(ctx, network, address)
-}

internal/engine/netx/dialer/timeout_test.go

@@ -1,34 +0,0 @@
-package dialer_test
-
-import (
-	"context"
-	"errors"
-	"io"
-	"net"
-	"testing"
-	"time"
-
-	"github.com/ooni/probe-cli/v3/internal/engine/netx/dialer"
-)
-
-type SlowDialer struct{}
-
-func (SlowDialer) DialContext(ctx context.Context, network, address string) (net.Conn, error) {
-	select {
-	case <-ctx.Done():
-		return nil, ctx.Err()
-	case <-time.After(30 * time.Second):
-		return nil, io.EOF
-	}
-}
-
-func TestTimeoutDialer(t *testing.T) {
-	d := dialer.TimeoutDialer{Dialer: SlowDialer{}, ConnectTimeout: time.Second}
-	conn, err := d.DialContext(context.Background(), "tcp", "www.google.com:443")
-	if !errors.Is(err, context.DeadlineExceeded) {
-		t.Fatal("not the error we expected")
-	}
-	if conn != nil {
-		t.Fatal("expected nil conn here")
-	}
-}

internal/engine/netx/netx.go

@@ -147,7 +147,6 @@ func NewDialer(config Config) Dialer {
 		config.FullResolver = NewResolver(config)
 	}
 	var d Dialer = dialer.Default
-	d = dialer.TimeoutDialer{Dialer: d}
 	d = dialer.ErrorWrapperDialer{Dialer: d}
 	if config.Logger != nil {
 		d = dialer.LoggingDialer{Dialer: d, Logger: config.Logger}

internal/engine/netx/netx_test.go

@@ -241,11 +241,7 @@ func TestNewDialerVanilla(t *testing.T) {
 	if !ok {
 		t.Fatal("not the dialer we expected")
 	}
-	td, ok := ewd.Dialer.(dialer.TimeoutDialer)
-	if !ok {
-		t.Fatal("not the dialer we expected")
-	}
-	if _, ok := td.Dialer.(dialer.SystemDialer); !ok {
+	if _, ok := ewd.Dialer.(dialer.SystemDialer); !ok {
 		t.Fatal("not the dialer we expected")
 	}
 }
@@ -281,11 +277,7 @@ func TestNewDialerWithResolver(t *testing.T) {
 	if !ok {
 		t.Fatal("not the dialer we expected")
 	}
-	td, ok := ewd.Dialer.(dialer.TimeoutDialer)
-	if !ok {
-		t.Fatal("not the dialer we expected")
-	}
-	if _, ok := td.Dialer.(dialer.SystemDialer); !ok {
+	if _, ok := ewd.Dialer.(dialer.SystemDialer); !ok {
 		t.Fatal("not the dialer we expected")
 	}
 }
@@ -330,11 +322,7 @@ func TestNewDialerWithLogger(t *testing.T) {
 	if !ok {
 		t.Fatal("not the dialer we expected")
 	}
-	td, ok := ewd.Dialer.(dialer.TimeoutDialer)
-	if !ok {
-		t.Fatal("not the dialer we expected")
-	}
-	if _, ok := td.Dialer.(dialer.SystemDialer); !ok {
+	if _, ok := ewd.Dialer.(dialer.SystemDialer); !ok {
 		t.Fatal("not the dialer we expected")
 	}
 }
@@ -380,11 +368,7 @@ func TestNewDialerWithDialSaver(t *testing.T) {
 	if !ok {
 		t.Fatal("not the dialer we expected")
 	}
-	td, ok := ewd.Dialer.(dialer.TimeoutDialer)
-	if !ok {
-		t.Fatal("not the dialer we expected")
-	}
-	if _, ok := td.Dialer.(dialer.SystemDialer); !ok {
+	if _, ok := ewd.Dialer.(dialer.SystemDialer); !ok {
 		t.Fatal("not the dialer we expected")
 	}
 }
@@ -430,11 +414,7 @@ func TestNewDialerWithReadWriteSaver(t *testing.T) {
 	if !ok {
 		t.Fatal("not the dialer we expected")
 	}
-	td, ok := ewd.Dialer.(dialer.TimeoutDialer)
-	if !ok {
-		t.Fatal("not the dialer we expected")
-	}
-	if _, ok := td.Dialer.(dialer.SystemDialer); !ok {
+	if _, ok := ewd.Dialer.(dialer.SystemDialer); !ok {
 		t.Fatal("not the dialer we expected")
 	}
 }
@@ -476,11 +456,7 @@ func TestNewDialerWithContextByteCounting(t *testing.T) {
 	if !ok {
 		t.Fatal("not the dialer we expected")
 	}
-	td, ok := ewd.Dialer.(dialer.TimeoutDialer)
-	if !ok {
-		t.Fatal("not the dialer we expected")
-	}
-	if _, ok := td.Dialer.(dialer.SystemDialer); !ok {
+	if _, ok := ewd.Dialer.(dialer.SystemDialer); !ok {
 		t.Fatal("not the dialer we expected")
 	}
 }