refactor(netx/dialer): we can simplify the proxy (#371)

The socks5 factory always returns a DialContext capable dialer. We just
need to cast to obtain such a dialer.

Also, the code will use the DialContext if passed a dialer that
implements DialContext.

Write a test that proves my point.

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

internal/engine/netx/dialer/proxy.go

@@ -17,6 +17,9 @@ type ProxyDialer struct {
 	ProxyURL *url.URL
 }
 
+// ErrProxyUnsupportedScheme indicates we don't support a protocol scheme.
+var ErrProxyUnsupportedScheme = errors.New("proxy: unsupported scheme")
+
 // DialContext implements Dialer.DialContext
 func (d ProxyDialer) DialContext(ctx context.Context, network, address string) (net.Conn, error) {
 	url := d.ProxyURL
@@ -24,38 +27,18 @@ func (d ProxyDialer) DialContext(ctx context.Context, network, address string) (
 		return d.Dialer.DialContext(ctx, network, address)
 	}
 	if url.Scheme != "socks5" {
-		return nil, errors.New("Scheme is not socks5")
+		return nil, ErrProxyUnsupportedScheme
 	}
 	// the code at proxy/socks5.go never fails; see https://git.io/JfJ4g
 	child, _ := proxy.SOCKS5(
-		network, url.Host, nil, proxyDialerWrapper{Dialer: d.Dialer})
+		network, url.Host, nil, proxyDialerWrapper{d.Dialer})
 	return d.dial(ctx, child, network, address)
 }
 
 func (d ProxyDialer) dial(
 	ctx context.Context, child proxy.Dialer, network, address string) (net.Conn, error) {
-	connch := make(chan net.Conn)
+	cd := child.(proxy.ContextDialer) // will work
-	errch := make(chan error, 1)
+	return cd.DialContext(ctx, network, address)
-	go func() {
-		conn, err := child.Dial(network, address)
-		if err != nil {
-			errch <- err
-			return
-		}
-		select {
-		case connch <- conn:
-		default:
-			conn.Close()
-		}
-	}()
-	select {
-	case <-ctx.Done():
-		return nil, ctx.Err()
-	case err := <-errch:
-		return nil, err
-	case conn := <-connch:
-		return conn, nil
-	}
 }
 
 // proxyDialerWrapper is required because SOCKS5 expects a Dialer.Dial type but internally
@@ -68,5 +51,5 @@ type proxyDialerWrapper struct {
 }
 
 func (d proxyDialerWrapper) Dial(network, address string) (net.Conn, error) {
-	return d.DialContext(context.Background(), network, address)
+	panic(errors.New("proxyDialerWrapper.Dial should not be called directly"))
 }

internal/engine/netx/dialer/proxy_internal_test.go

@@ -1,15 +0,0 @@
-package dialer
-
-import (
-	"context"
-	"net"
-
-	"golang.org/x/net/proxy"
-)
-
-type ProxyDialerWrapper = proxyDialerWrapper
-
-func (d ProxyDialer) DialContextWithDialer(
-	ctx context.Context, child proxy.Dialer, network, address string) (net.Conn, error) {
-	return d.dial(ctx, child, network, address)
-}

internal/engine/netx/dialer/proxy_test.go

@@ -1,4 +1,4 @@
-package dialer_test
+package dialer
 
 import (
 	"context"
@@ -7,15 +7,13 @@ import (
 	"net"
 	"net/url"
 	"testing"
-	"time"
 
-	"github.com/ooni/probe-cli/v3/internal/engine/netx/dialer"
 	"github.com/ooni/probe-cli/v3/internal/engine/netx/mockablex"
 )
 
 func TestProxyDialerDialContextNoProxyURL(t *testing.T) {
 	expected := errors.New("mocked error")
-	d := dialer.ProxyDialer{
+	d := ProxyDialer{
 		Dialer: mockablex.Dialer{
 			MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) {
 				return nil, expected
@@ -32,11 +30,11 @@ func TestProxyDialerDialContextNoProxyURL(t *testing.T) {
 }
 
 func TestProxyDialerDialContextInvalidScheme(t *testing.T) {
-	d := dialer.ProxyDialer{
+	d := ProxyDialer{
 		ProxyURL: &url.URL{Scheme: "antani"},
 	}
 	conn, err := d.DialContext(context.Background(), "tcp", "www.google.com:443")
-	if err.Error() != "Scheme is not socks5" {
+	if !errors.Is(err, ErrProxyUnsupportedScheme) {
 		t.Fatal("not the error we expected")
 	}
 	if conn != nil {
@@ -45,13 +43,17 @@ func TestProxyDialerDialContextInvalidScheme(t *testing.T) {
 }
 
 func TestProxyDialerDialContextWithEOF(t *testing.T) {
-	d := dialer.ProxyDialer{
+	const expect = "10.0.0.1:9050"
+	d := ProxyDialer{
 		Dialer: mockablex.Dialer{
 			MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) {
+				if address != expect {
+					return nil, errors.New("unexpected address")
+				}
 				return nil, io.EOF
 			},
 		},
-		ProxyURL: &url.URL{Scheme: "socks5"},
+		ProxyURL: &url.URL{Scheme: "socks5", Host: expect},
 	}
 	conn, err := d.DialContext(context.Background(), "tcp", "www.google.com:443")
 	if !errors.Is(err, io.EOF) {
@@ -62,105 +64,18 @@ func TestProxyDialerDialContextWithEOF(t *testing.T) {
 	}
 }
 
-func TestProxyDialerDialContextWithContextCanceled(t *testing.T) {
+func TestProxyDialWrapperPanics(t *testing.T) {
-	ctx, cancel := context.WithCancel(context.Background())
+	d := &proxyDialerWrapper{}
-	cancel() // immediately fail
+	err := func() (rv error) {
-	d := dialer.ProxyDialer{
+		defer func() {
-		Dialer: mockablex.Dialer{
+			if r := recover(); r != nil {
-			MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) {
+				rv = r.(error)
-				return nil, io.EOF
-			},
-		},
-		ProxyURL: &url.URL{Scheme: "socks5"},
 			}
-	conn, err := d.DialContext(ctx, "tcp", "www.google.com:443")
+		}()
-	if !errors.Is(err, context.Canceled) {
+		d.Dial("tcp", "10.0.0.1:1234")
-		t.Fatal("not the error we expected")
+		return
-	}
+	}()
-	if conn != nil {
+	if err.Error() != "proxyDialerWrapper.Dial should not be called directly" {
-		t.Fatal("conn is not nil")
+		t.Fatal("unexpected result", err)
-	}
-}
-
-func TestProxyDialerDialContextWithDialerSuccess(t *testing.T) {
-	d := dialer.ProxyDialer{
-		Dialer: mockablex.Dialer{
-			MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) {
-				return &mockablex.Conn{
-					MockRead: func(b []byte) (int, error) {
-						return 0, io.EOF
-					},
-					MockWrite: func(b []byte) (int, error) {
-						return 0, io.EOF
-					},
-					MockClose: func() error {
-						return io.EOF
-					},
-				}, nil
-			},
-		},
-		ProxyURL: &url.URL{Scheme: "socks5"},
-	}
-	conn, err := d.DialContextWithDialer(
-		context.Background(), dialer.ProxyDialerWrapper{
-			Dialer: d.Dialer,
-		}, "tcp", "www.google.com:443")
-	if err != nil {
-		t.Fatal(err)
-	}
-	conn.Close()
-}
-
-func TestProxyDialerDialContextWithDialerCanceledContext(t *testing.T) {
-	ctx, cancel := context.WithCancel(context.Background())
-	// Stop immediately. The FakeDialer sleeps for some microseconds so
-	// it is much more likely we immediately exit with done context. The
-	// arm where we receive the conn is much less likely.
-	cancel()
-	d := dialer.ProxyDialer{
-		Dialer: mockablex.Dialer{
-			MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) {
-				time.Sleep(10 * time.Microsecond)
-				return &mockablex.Conn{
-					MockRead: func(b []byte) (int, error) {
-						return 0, io.EOF
-					},
-					MockWrite: func(b []byte) (int, error) {
-						return 0, io.EOF
-					},
-					MockClose: func() error {
-						return io.EOF
-					},
-				}, nil
-			},
-		},
-		ProxyURL: &url.URL{Scheme: "socks5"},
-	}
-	conn, err := d.DialContextWithDialer(
-		ctx, dialer.ProxyDialerWrapper{
-			Dialer: d.Dialer,
-		}, "tcp", "www.google.com:443")
-	if !errors.Is(err, context.Canceled) {
-		t.Fatal("not the error we expected")
-	}
-	if conn != nil {
-		t.Fatal("expected nil conn here")
-	}
-}
-
-func TestProxyDialerWrapper(t *testing.T) {
-	d := dialer.ProxyDialerWrapper{
-		Dialer: mockablex.Dialer{
-			MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) {
-				return nil, io.EOF
-			},
-		},
-	}
-	conn, err := d.Dial("tcp", "www.google.com:443")
-	if !errors.Is(err, io.EOF) {
-		t.Fatal("not the error we expected")
-	}
-	if conn != nil {
-		t.Fatal("conn is not nil")
 	}
 }