fix(netxlite): prefer composition over embedding (#733)

This diff has been extracted and adapted from 8848c8c516

The reason to prefer composition over embedding is that we want the
build to break if we add new methods to interfaces we define. If the build
does not break, we may forget about wrapping methods we should
actually be wrapping. I noticed this issue inside netxlite when I was working
on websteps-illustrated and I added support for NS and PTR queries.

See https://github.com/ooni/probe/issues/2096

While there, perform comprehensive netxlite code review
and apply minor changes and improve the docs.

internal/netxlite/bogon.go

@@ -18,7 +18,7 @@ import (
 // function a non-IP address causes it to return true.
 func IsBogon(address string) bool {
 	ip := net.ParseIP(address)
-	return ip == nil || isPrivate(address, ip)
+	return ip == nil || isBogon(address, ip)
 }
 
 // IsLoopback returns whether an IP address is loopback. Passing to this
@@ -33,7 +33,7 @@ var (
 	bogons6 []*net.IPNet
 )
 
-func expandbogons(cidrs []string) (out []*net.IPNet) {
+func expandBogons(cidrs []string) (out []*net.IPNet) {
 	for _, cidr := range cidrs {
 		_, block, err := net.ParseCIDR(cidr)
 		runtimex.PanicOnError(err, "net.ParseCIDR failed")
@@ -43,7 +43,7 @@ func expandbogons(cidrs []string) (out []*net.IPNet) {
 }
 
 func init() {
-	bogons4 = append(bogons4, expandbogons([]string{
+	bogons4 = append(bogons4, expandBogons([]string{
 		//
 		// List extracted from https://ipinfo.io/bogon
 		//
@@ -64,7 +64,7 @@ func init() {
 		"240.0.0.0/4",        // Reserved for future use
 		"255.255.255.255/32", // Limited broadcast
 	})...)
-	bogons6 = append(bogons6, expandbogons([]string{
+	bogons6 = append(bogons6, expandBogons([]string{
 		//
 		// List extracted from https://ipinfo.io/bogon
 		//
@@ -110,7 +110,10 @@ func init() {
 	})...)
 }
 
-func isPrivate(address string, ip net.IP) bool {
+// isBogon implements IsBogon
+func isBogon(address string, ip net.IP) bool {
+	// TODO(bassosimone): the following check is probably redundant given that these
+	// three checks are already included into the list of bogons.
 	if ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() {
 		return true
 	}

internal/netxlite/classify.go

@@ -1,5 +1,9 @@
 package netxlite
 
+//
+// Mapping Go errors to OONI errors
+//
+
 import (
 	"context"
 	"crypto/x509"
@@ -38,8 +42,7 @@ func classifyGenericError(err error) string {
 	// The list returned here matches the values used by MK unless
 	// explicitly noted otherwise with a comment.
 
-	// QUIRK: we cannot remove this check as long as this function
+	// Robustness: handle the case where we're passed a wrapped error.
-	// is exported and used independently from NewErrWrapper.
 	var errwrapper *ErrWrapper
 	if errors.As(err, &errwrapper) {
 		return errwrapper.Error() // we've already wrapped it
@@ -146,8 +149,7 @@ const (
 // and returns to the caller its return value.
 func classifyQUICHandshakeError(err error) string {
 
-	// QUIRK: we cannot remove this check as long as this function
+	// Robustness: handle the case where we're passed a wrapped error.
-	// is exported and used independently from NewErrWrapper.
 	var errwrapper *ErrWrapper
 	if errors.As(err, &errwrapper) {
 		return errwrapper.Error() // we've already wrapped it
@@ -269,8 +271,7 @@ var (
 // returns to the caller its return value.
 func classifyResolverError(err error) string {
 
-	// QUIRK: we cannot remove this check as long as this function
+	// Robustness: handle the case where we're passed a wrapped error.
-	// is exported and used independently from NewErrWrapper.
 	var errwrapper *ErrWrapper
 	if errors.As(err, &errwrapper) {
 		return errwrapper.Error() // we've already wrapped it
@@ -303,8 +304,7 @@ func classifyResolverError(err error) string {
 // returns to the caller its return value.
 func classifyTLSHandshakeError(err error) string {
 
-	// QUIRK: we cannot remove this check as long as this function
+	// Robustness: handle the case where we're passed a wrapped error.
-	// is exported and used independently from NewErrWrapper.
 	var errwrapper *ErrWrapper
 	if errors.As(err, &errwrapper) {
 		return errwrapper.Error() // we've already wrapped it

internal/netxlite/dialer.go

@@ -1,5 +1,9 @@
 package netxlite
 
+//
+// Code for dialing TCP or UDP net.Conn-like connections
+//
+
 import (
 	"context"
 	"errors"
@@ -96,8 +100,8 @@ func (d *dialerSystem) CloseIdleConnections() {
 
 // dialerResolver combines dialing with domain name resolution.
 type dialerResolver struct {
-	model.Dialer
+	Dialer   model.Dialer
-	model.Resolver
+	Resolver model.Resolver
 }
 
 var _ model.Dialer = &dialerResolver{}
@@ -144,10 +148,10 @@ func (d *dialerResolver) CloseIdleConnections() {
 // dialerLogger is a Dialer with logging.
 type dialerLogger struct {
 	// Dialer is the underlying dialer.
-	model.Dialer
+	Dialer model.Dialer
 
-	// Logger is the underlying logger.
+	// DebugLogger is the underlying logger.
-	model.DebugLogger
+	DebugLogger model.DebugLogger
 
 	// operationSuffix is appended to the operation name.
 	//
@@ -194,15 +198,15 @@ func NewSingleUseDialer(conn net.Conn) model.Dialer {
 
 // dialerSingleUse is the Dialer returned by NewSingleDialer.
 type dialerSingleUse struct {
-	sync.Mutex
+	mu   sync.Mutex
 	conn net.Conn
 }
 
 var _ model.Dialer = &dialerSingleUse{}
 
 func (s *dialerSingleUse) DialContext(ctx context.Context, network string, addr string) (net.Conn, error) {
-	defer s.Unlock()
+	defer s.mu.Unlock()
-	s.Lock()
+	s.mu.Lock()
 	if s.conn == nil {
 		return nil, ErrNoConnReuse
 	}
@@ -218,7 +222,7 @@ func (s *dialerSingleUse) CloseIdleConnections() {
 // dialerErrWrapper is a dialer that performs error wrapping. The connection
 // returned by the DialContext function will also perform error wrapping.
 type dialerErrWrapper struct {
-	model.Dialer
+	Dialer model.Dialer
 }
 
 var _ model.Dialer = &dialerErrWrapper{}
@@ -226,11 +230,15 @@ var _ model.Dialer = &dialerErrWrapper{}
 func (d *dialerErrWrapper) DialContext(ctx context.Context, network, address string) (net.Conn, error) {
 	conn, err := d.Dialer.DialContext(ctx, network, address)
 	if err != nil {
-		return nil, NewErrWrapper(classifyGenericError, ConnectOperation, err)
+		return nil, newErrWrapper(classifyGenericError, ConnectOperation, err)
 	}
 	return &dialerErrWrapperConn{Conn: conn}, nil
 }
 
+func (d *dialerErrWrapper) CloseIdleConnections() {
+	d.Dialer.CloseIdleConnections()
+}
+
 // dialerErrWrapperConn is a net.Conn that performs error wrapping.
 type dialerErrWrapperConn struct {
 	net.Conn
@@ -241,7 +249,7 @@ var _ net.Conn = &dialerErrWrapperConn{}
 func (c *dialerErrWrapperConn) Read(b []byte) (int, error) {
 	count, err := c.Conn.Read(b)
 	if err != nil {
-		return 0, NewErrWrapper(classifyGenericError, ReadOperation, err)
+		return 0, newErrWrapper(classifyGenericError, ReadOperation, err)
 	}
 	return count, nil
 }
@@ -249,7 +257,7 @@ func (c *dialerErrWrapperConn) Read(b []byte) (int, error) {
 func (c *dialerErrWrapperConn) Write(b []byte) (int, error) {
 	count, err := c.Conn.Write(b)
 	if err != nil {
-		return 0, NewErrWrapper(classifyGenericError, WriteOperation, err)
+		return 0, newErrWrapper(classifyGenericError, WriteOperation, err)
 	}
 	return count, nil
 }
@@ -257,7 +265,7 @@ func (c *dialerErrWrapperConn) Write(b []byte) (int, error) {
 func (c *dialerErrWrapperConn) Close() error {
 	err := c.Conn.Close()
 	if err != nil {
-		return NewErrWrapper(classifyGenericError, CloseOperation, err)
+		return newErrWrapper(classifyGenericError, CloseOperation, err)
 	}
 	return nil
 }

internal/netxlite/dialer_test.go

@@ -253,7 +253,7 @@ func TestDialerResolver(t *testing.T) {
 			mu := &sync.Mutex{}
 			errorsList := []error{
 				errors.New("a mocked error"),
-				NewErrWrapper(
+				newErrWrapper(
 					classifyGenericError,
 					CloseOperation,
 					io.EOF,
@@ -295,7 +295,7 @@ func TestDialerResolver(t *testing.T) {
 			mu := &sync.Mutex{}
 			errorsList := []error{
 				errors.New("a mocked error"),
-				NewErrWrapper(
+				newErrWrapper(
 					classifyGenericError,
 					CloseOperation,
 					errors.New("antani"),

internal/netxlite/dnsdecoder.go

@@ -1,5 +1,9 @@
 package netxlite
 
+//
+// Decode byte arrays to DNS messages
+//
+
 import (
 	"errors"
 

internal/netxlite/dnsencoder.go

@@ -1,5 +1,9 @@
 package netxlite
 
+//
+// Encode DNS queries to byte arrays
+//
+
 import (
 	"github.com/miekg/dns"
 	"github.com/ooni/probe-cli/v3/internal/model"

internal/netxlite/dnsoverhttps.go

@@ -1,5 +1,9 @@
 package netxlite
 
+//
+// DNS-over-HTTPS transport
+//
+
 import (
 	"bytes"
 	"context"

internal/netxlite/dnsovertcp.go

@@ -1,5 +1,9 @@
 package netxlite
 
+//
+// DNS-over-{TCP,TLS} transport
+//
+
 import (
 	"context"
 	"errors"

internal/netxlite/dnsoverudp.go

@@ -1,5 +1,9 @@
 package netxlite
 
+//
+// DNS-over-UDP transport
+//
+
 import (
 	"context"
 	"time"

internal/netxlite/dnstransport.go

@@ -1 +0,0 @@
-package netxlite

internal/netxlite/doc.go

@@ -3,7 +3,7 @@
 // This package is the basic networking building block that you
 // should be using every time you need networking.
 //
-// It implements interfaces defined in the internal/model package.
+// It implements interfaces defined in internal/model/netx.go.
 //
 // You should consider checking the tutorial explaining how to use this package
 // for network measurements: https://github.com/ooni/probe-cli/tree/master/internal/tutorial/netxlite.

internal/netxlite/errwrapper.go

@@ -66,12 +66,12 @@ func (e *ErrWrapper) MarshalJSON() ([]byte, error) {
 	return json.Marshal(e.Failure)
 }
 
-// Classifier is the type of the function that maps a Go error
+// classifier is the type of the function that maps a Go error
 // to a OONI failure string defined at
 // https://github.com/ooni/spec/blob/master/data-formats/df-007-errors.md.
-type Classifier func(err error) string
+type classifier func(err error) string
 
-// NewErrWrapper creates a new ErrWrapper using the given
+// newErrWrapper creates a new ErrWrapper using the given
 // classifier, operation name, and underlying error.
 //
 // This function panics if classifier is nil, or operation
@@ -81,7 +81,7 @@ type Classifier func(err error) string
 // error wrapper will use the same classification string and
 // will determine whether to keep the major operation as documented
 // in the ErrWrapper.Operation documentation.
-func NewErrWrapper(c Classifier, op string, err error) *ErrWrapper {
+func newErrWrapper(c classifier, op string, err error) *ErrWrapper {
 	var wrapper *ErrWrapper
 	if errors.As(err, &wrapper) {
 		return &ErrWrapper{
@@ -107,13 +107,15 @@ func NewErrWrapper(c Classifier, op string, err error) *ErrWrapper {
 }
 
 // NewTopLevelGenericErrWrapper wraps an error occurring at top
-// level using ClassifyGenericError as classifier.
+// level using a generic classifier as classifier. This is the
+// function you should call when you suspect a given error hasn't
+// already been wrapped. This function panics if err is nil.
 //
 // If the err argument has already been classified, the returned
 // error wrapper will use the same classification string and
 // failed operation of the original error.
 func NewTopLevelGenericErrWrapper(err error) *ErrWrapper {
-	return NewErrWrapper(classifyGenericError, TopLevelOperation, err)
+	return newErrWrapper(classifyGenericError, TopLevelOperation, err)
 }
 
 func classifyOperation(ew *ErrWrapper, operation string) string {

internal/netxlite/errwrapper_test.go

@@ -53,7 +53,7 @@ func TestNewErrWrapper(t *testing.T) {
 					recovered.Add(1)
 				}
 			}()
-			NewErrWrapper(nil, CloseOperation, io.EOF)
+			newErrWrapper(nil, CloseOperation, io.EOF)
 		}()
 		if recovered.Load() != 1 {
 			t.Fatal("did not panic")
@@ -68,7 +68,7 @@ func TestNewErrWrapper(t *testing.T) {
 					recovered.Add(1)
 				}
 			}()
-			NewErrWrapper(classifyGenericError, "", io.EOF)
+			newErrWrapper(classifyGenericError, "", io.EOF)
 		}()
 		if recovered.Load() != 1 {
 			t.Fatal("did not panic")
@@ -83,7 +83,7 @@ func TestNewErrWrapper(t *testing.T) {
 					recovered.Add(1)
 				}
 			}()
-			NewErrWrapper(classifyGenericError, CloseOperation, nil)
+			newErrWrapper(classifyGenericError, CloseOperation, nil)
 		}()
 		if recovered.Load() != 1 {
 			t.Fatal("did not panic")
@@ -91,7 +91,7 @@ func TestNewErrWrapper(t *testing.T) {
 	})
 
 	t.Run("otherwise, works as intended", func(t *testing.T) {
-		ew := NewErrWrapper(classifyGenericError, CloseOperation, io.EOF)
+		ew := newErrWrapper(classifyGenericError, CloseOperation, io.EOF)
 		if ew.Failure != FailureEOFError {
 			t.Fatal("unexpected failure")
 		}
@@ -104,10 +104,10 @@ func TestNewErrWrapper(t *testing.T) {
 	})
 
 	t.Run("when the underlying error is already a wrapped error", func(t *testing.T) {
-		ew := NewErrWrapper(classifySyscallError, ReadOperation, ECONNRESET)
+		ew := newErrWrapper(classifySyscallError, ReadOperation, ECONNRESET)
 		var err1 error = ew
 		err2 := fmt.Errorf("cannot read: %w", err1)
-		ew2 := NewErrWrapper(classifyGenericError, HTTPRoundTripOperation, err2)
+		ew2 := newErrWrapper(classifyGenericError, HTTPRoundTripOperation, err2)
 		if ew2.Failure != ew.Failure {
 			t.Fatal("not the same failure")
 		}

internal/netxlite/http.go

@@ -1,5 +1,9 @@
 package netxlite
 
+//
+// HTTP/1.1 and HTTP2 code
+//
+
 import (
 	"context"
 	"errors"
@@ -13,9 +17,11 @@ import (
 
 // httpTransportErrWrapper is an HTTPTransport with error wrapping.
 type httpTransportErrWrapper struct {
-	model.HTTPTransport
+	HTTPTransport model.HTTPTransport
 }
 
+var _ model.HTTPTransport = &httpTransportErrWrapper{}
+
 func (txp *httpTransportErrWrapper) RoundTrip(req *http.Request) (*http.Response, error) {
 	resp, err := txp.HTTPTransport.RoundTrip(req)
 	if err != nil {
@@ -24,10 +30,18 @@ func (txp *httpTransportErrWrapper) RoundTrip(req *http.Request) (*http.Response
 	return resp, nil
 }
 
+func (txp *httpTransportErrWrapper) CloseIdleConnections() {
+	txp.HTTPTransport.CloseIdleConnections()
+}
+
+func (txp *httpTransportErrWrapper) Network() string {
+	return txp.HTTPTransport.Network()
+}
+
 // httpTransportLogger is an HTTPTransport with logging.
 type httpTransportLogger struct {
 	// HTTPTransport is the underlying HTTP transport.
-	model.HTTPTransport
+	HTTPTransport model.HTTPTransport
 
 	// Logger is the underlying logger.
 	Logger model.DebugLogger
@@ -62,12 +76,26 @@ func (txp *httpTransportLogger) CloseIdleConnections() {
 	txp.HTTPTransport.CloseIdleConnections()
 }
 
+func (txp *httpTransportLogger) Network() string {
+	return txp.HTTPTransport.Network()
+}
+
 // httpTransportConnectionsCloser is an HTTPTransport that
 // correctly forwards CloseIdleConnections calls.
 type httpTransportConnectionsCloser struct {
-	model.HTTPTransport
+	HTTPTransport model.HTTPTransport
-	model.Dialer
+	Dialer        model.Dialer
-	model.TLSDialer
+	TLSDialer     model.TLSDialer
+}
+
+var _ model.HTTPTransport = &httpTransportConnectionsCloser{}
+
+func (txp *httpTransportConnectionsCloser) RoundTrip(req *http.Request) (*http.Response, error) {
+	return txp.HTTPTransport.RoundTrip(req)
+}
+
+func (txp *httpTransportConnectionsCloser) Network() string {
+	return txp.HTTPTransport.Network()
 }
 
 // CloseIdleConnections forwards the CloseIdleConnections calls.
@@ -148,7 +176,17 @@ func NewOOHTTPBaseTransport(dialer model.Dialer, tlsDialer model.TLSDialer) mode
 
 // stdlibTransport wraps oohttp.StdlibTransport to add .Network()
 type stdlibTransport struct {
-	*oohttp.StdlibTransport
+	StdlibTransport *oohttp.StdlibTransport
+}
+
+var _ model.HTTPTransport = &stdlibTransport{}
+
+func (txp *stdlibTransport) CloseIdleConnections() {
+	txp.StdlibTransport.CloseIdleConnections()
+}
+
+func (txp *stdlibTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	return txp.StdlibTransport.RoundTrip(req)
 }
 
 // Network implements HTTPTransport.Network.
@@ -170,7 +208,13 @@ func WrapHTTPTransport(logger model.DebugLogger, txp model.HTTPTransport) model.
 // httpDialerWithReadTimeout enforces a read timeout for all HTTP
 // connections. See https://github.com/ooni/probe/issues/1609.
 type httpDialerWithReadTimeout struct {
-	model.Dialer
+	Dialer model.Dialer
+}
+
+var _ model.Dialer = &httpDialerWithReadTimeout{}
+
+func (d *httpDialerWithReadTimeout) CloseIdleConnections() {
+	d.Dialer.CloseIdleConnections()
 }
 
 // DialContext implements Dialer.DialContext.
@@ -186,7 +230,13 @@ func (d *httpDialerWithReadTimeout) DialContext(
 // httpTLSDialerWithReadTimeout enforces a read timeout for all HTTP
 // connections. See https://github.com/ooni/probe/issues/1609.
 type httpTLSDialerWithReadTimeout struct {
-	model.TLSDialer
+	TLSDialer model.TLSDialer
+}
+
+var _ model.TLSDialer = &httpTLSDialerWithReadTimeout{}
+
+func (d *httpTLSDialerWithReadTimeout) CloseIdleConnections() {
+	d.TLSDialer.CloseIdleConnections()
 }
 
 // ErrNotTLSConn occur when an interface accepts a net.Conn but
@@ -280,7 +330,7 @@ func WrapHTTPClient(clnt model.HTTPClient) model.HTTPClient {
 }
 
 type httpClientErrWrapper struct {
-	model.HTTPClient
+	HTTPClient model.HTTPClient
 }
 
 func (c *httpClientErrWrapper) Do(req *http.Request) (*http.Response, error) {
@@ -290,3 +340,7 @@ func (c *httpClientErrWrapper) Do(req *http.Request) (*http.Response, error) {
 	}
 	return resp, nil
 }
+
+func (c *httpClientErrWrapper) CloseIdleConnections() {
+	c.HTTPClient.CloseIdleConnections()
+}

internal/netxlite/http3.go

@@ -1,5 +1,9 @@
 package netxlite
 
+//
+// HTTP3 code
+//
+
 import (
 	"crypto/tls"
 	"io"

internal/netxlite/http_test.go

@@ -252,19 +252,19 @@ func TestNewHTTPTransport(t *testing.T) {
 			t.Fatal("invalid tls dialer")
 		}
 		stdlib := connectionsCloser.HTTPTransport.(*stdlibTransport)
-		if !stdlib.Transport.ForceAttemptHTTP2 {
+		if !stdlib.StdlibTransport.ForceAttemptHTTP2 {
 			t.Fatal("invalid ForceAttemptHTTP2")
 		}
-		if !stdlib.Transport.DisableCompression {
+		if !stdlib.StdlibTransport.DisableCompression {
 			t.Fatal("invalid DisableCompression")
 		}
-		if stdlib.Transport.MaxConnsPerHost != 1 {
+		if stdlib.StdlibTransport.MaxConnsPerHost != 1 {
 			t.Fatal("invalid MaxConnPerHost")
 		}
-		if stdlib.Transport.DialTLSContext == nil {
+		if stdlib.StdlibTransport.DialTLSContext == nil {
 			t.Fatal("invalid DialTLSContext")
 		}
-		if stdlib.Transport.DialContext == nil {
+		if stdlib.StdlibTransport.DialContext == nil {
 			t.Fatal("invalid DialContext")
 		}
 	})
@@ -500,6 +500,20 @@ func TestHTTPClientErrWrapper(t *testing.T) {
 			}
 		})
 	})
+
+	t.Run("CloseIdleConnections", func(t *testing.T) {
+		var called bool
+		child := &mocks.HTTPClient{
+			MockCloseIdleConnections: func() {
+				called = true
+			},
+		}
+		clnt := &httpClientErrWrapper{child}
+		clnt.CloseIdleConnections()
+		if !called {
+			t.Fatal("not called")
+		}
+	})
 }
 
 func TestNewHTTPClientStdlib(t *testing.T) {

internal/netxlite/iox.go

@@ -1,5 +1,9 @@
 package netxlite
 
+//
+// I/O extensions
+//
+
 import (
 	"context"
 	"errors"

internal/netxlite/iox_test.go

@@ -41,7 +41,7 @@ func TestReadAllContext(t *testing.T) {
 				//
 				// Note: Returning a wrapped error to ensure we address
 				// https://github.com/ooni/probe/issues/1965
-				return len(b), NewErrWrapper(classifyGenericError,
+				return len(b), newErrWrapper(classifyGenericError,
 					ReadOperation, io.EOF)
 			},
 		}
@@ -171,7 +171,7 @@ func TestCopyContext(t *testing.T) {
 				//
 				// Note: Returning a wrapped error to ensure we address
 				// https://github.com/ooni/probe/issues/1965
-				return len(b), NewErrWrapper(classifyGenericError,
+				return len(b), newErrWrapper(classifyGenericError,
 					ReadOperation, io.EOF)
 			},
 		}

internal/netxlite/legacy.go

@@ -1,5 +1,9 @@
 package netxlite
 
+//
+// Legacy code
+//
+
 // These vars export internal names to legacy ooni/probe-cli code.
 //
 // Deprecated: do not use these names in new code.

internal/netxlite/operations.go

@@ -1,5 +1,9 @@
 package netxlite
 
+//
+// Names of operations
+//
+
 // Operations that we measure. They are the possible values of
 // the ErrWrapper.Operation field.
 const (

internal/netxlite/parallelresolver.go

@@ -1,7 +1,7 @@
 package netxlite
 
 //
-// Parallel resolver implementation
+// Parallel DNS resolver implementation
 //
 
 import (

internal/netxlite/quic.go

@@ -1,5 +1,9 @@
 package netxlite
 
+//
+// QUIC implementation
+//
+
 import (
 	"context"
 	"crypto/tls"
@@ -340,7 +344,7 @@ func NewSingleUseQUICDialer(qconn quic.EarlyConnection) model.QUICDialer {
 
 // quicDialerSingleUse is the QUICDialer returned by NewSingleQUICDialer.
 type quicDialerSingleUse struct {
-	sync.Mutex
+	mu    sync.Mutex
 	qconn quic.EarlyConnection
 }
 
@@ -351,8 +355,8 @@ func (s *quicDialerSingleUse) DialContext(
 	ctx context.Context, network, addr string, tlsCfg *tls.Config,
 	cfg *quic.Config) (quic.EarlyConnection, error) {
 	var qconn quic.EarlyConnection
-	defer s.Unlock()
+	defer s.mu.Unlock()
-	s.Lock()
+	s.mu.Lock()
 	if s.qconn == nil {
 		return nil, ErrNoConnReuse
 	}
@@ -368,7 +372,7 @@ func (s *quicDialerSingleUse) CloseIdleConnections() {
 // quicListenerErrWrapper is a QUICListener that wraps errors.
 type quicListenerErrWrapper struct {
 	// QUICListener is the underlying listener.
-	model.QUICListener
+	QUICListener model.QUICListener
 }
 
 var _ model.QUICListener = &quicListenerErrWrapper{}
@@ -377,7 +381,7 @@ var _ model.QUICListener = &quicListenerErrWrapper{}
 func (qls *quicListenerErrWrapper) Listen(addr *net.UDPAddr) (model.UDPLikeConn, error) {
 	pconn, err := qls.QUICListener.Listen(addr)
 	if err != nil {
-		return nil, NewErrWrapper(classifyGenericError, QUICListenOperation, err)
+		return nil, newErrWrapper(classifyGenericError, QUICListenOperation, err)
 	}
 	return &quicErrWrapperUDPLikeConn{pconn}, nil
 }
@@ -394,7 +398,7 @@ var _ model.UDPLikeConn = &quicErrWrapperUDPLikeConn{}
 func (c *quicErrWrapperUDPLikeConn) WriteTo(p []byte, addr net.Addr) (int, error) {
 	count, err := c.UDPLikeConn.WriteTo(p, addr)
 	if err != nil {
-		return 0, NewErrWrapper(classifyGenericError, WriteToOperation, err)
+		return 0, newErrWrapper(classifyGenericError, WriteToOperation, err)
 	}
 	return count, nil
 }
@@ -403,7 +407,7 @@ func (c *quicErrWrapperUDPLikeConn) WriteTo(p []byte, addr net.Addr) (int, error
 func (c *quicErrWrapperUDPLikeConn) ReadFrom(b []byte) (int, net.Addr, error) {
 	n, addr, err := c.UDPLikeConn.ReadFrom(b)
 	if err != nil {
-		return 0, nil, NewErrWrapper(classifyGenericError, ReadFromOperation, err)
+		return 0, nil, newErrWrapper(classifyGenericError, ReadFromOperation, err)
 	}
 	return n, addr, nil
 }
@@ -412,24 +416,30 @@ func (c *quicErrWrapperUDPLikeConn) ReadFrom(b []byte) (int, net.Addr, error) {
 func (c *quicErrWrapperUDPLikeConn) Close() error {
 	err := c.UDPLikeConn.Close()
 	if err != nil {
-		return NewErrWrapper(classifyGenericError, ReadFromOperation, err)
+		return newErrWrapper(classifyGenericError, ReadFromOperation, err)
 	}
 	return nil
 }
 
 // quicDialerErrWrapper is a dialer that performs quic err wrapping
 type quicDialerErrWrapper struct {
-	model.QUICDialer
+	QUICDialer model.QUICDialer
 }
 
+var _ model.QUICDialer = &quicDialerErrWrapper{}
+
 // DialContext implements ContextDialer.DialContext
 func (d *quicDialerErrWrapper) DialContext(
 	ctx context.Context, network string, host string,
 	tlsCfg *tls.Config, cfg *quic.Config) (quic.EarlyConnection, error) {
 	qconn, err := d.QUICDialer.DialContext(ctx, network, host, tlsCfg, cfg)
 	if err != nil {
-		return nil, NewErrWrapper(
+		return nil, newErrWrapper(
 			classifyQUICHandshakeError, QUICHandshakeOperation, err)
 	}
 	return qconn, nil
 }
+
+func (d *quicDialerErrWrapper) CloseIdleConnections() {
+	d.QUICDialer.CloseIdleConnections()
+}

internal/netxlite/quirks.go

@@ -1,15 +1,17 @@
 package netxlite
 
+//
+// This file contains weird stuff that we carried over from
+// the original netx implementation and that we cannot remove
+// or change without thinking about the consequences.
+//
+
 import (
 	"errors"
 	"net"
 	"strings"
 )
 
-// This file contains weird stuff that we carried over from
-// the original netx implementation and that we cannot remove
-// or change without thinking about the consequences.
-
 // See https://github.com/ooni/probe/issues/1985
 var errReduceErrorsEmptyList = errors.New("bug: reduceErrors given an empty list")
 

internal/netxlite/quirks_test.go

@@ -34,12 +34,12 @@ func TestQuirkReduceErrors(t *testing.T) {
 
 	t.Run("multiple errors with meaningful ones", func(t *testing.T) {
 		err1 := errors.New("mocked error #1")
-		err2 := NewErrWrapper(
+		err2 := newErrWrapper(
 			classifyGenericError,
 			CloseOperation,
 			errors.New("antani"),
 		)
-		err3 := NewErrWrapper(
+		err3 := newErrWrapper(
 			classifyGenericError,
 			CloseOperation,
 			ECONNREFUSED,

internal/netxlite/resolver.go

@@ -1,5 +1,9 @@
 package netxlite
 
+//
+// DNS resolver
+//
+
 import (
 	"context"
 	"errors"
@@ -134,8 +138,8 @@ func (r *resolverSystem) LookupHTTPS(
 
 // resolverLogger is a resolver that emits events
 type resolverLogger struct {
-	model.Resolver
+	Resolver model.Resolver
-	Logger model.DebugLogger
+	Logger   model.DebugLogger
 }
 
 var _ model.Resolver = &resolverLogger{}
@@ -172,13 +176,27 @@ func (r *resolverLogger) LookupHTTPS(
 	return https, nil
 }
 
+func (r *resolverLogger) Address() string {
+	return r.Resolver.Address()
+}
+
+func (r *resolverLogger) Network() string {
+	return r.Resolver.Network()
+}
+
+func (r *resolverLogger) CloseIdleConnections() {
+	r.Resolver.CloseIdleConnections()
+}
+
 // resolverIDNA supports resolving Internationalized Domain Names.
 //
 // See RFC3492 for more information.
 type resolverIDNA struct {
-	model.Resolver
+	Resolver model.Resolver
 }
 
+var _ model.Resolver = &resolverIDNA{}
+
 func (r *resolverIDNA) LookupHost(ctx context.Context, hostname string) ([]string, error) {
 	host, err := idna.ToASCII(hostname)
 	if err != nil {
@@ -196,12 +214,26 @@ func (r *resolverIDNA) LookupHTTPS(
 	return r.Resolver.LookupHTTPS(ctx, host)
 }
 
+func (r *resolverIDNA) Network() string {
+	return r.Resolver.Network()
+}
+
+func (r *resolverIDNA) Address() string {
+	return r.Resolver.Address()
+}
+
+func (r *resolverIDNA) CloseIdleConnections() {
+	r.Resolver.CloseIdleConnections()
+}
+
 // resolverShortCircuitIPAddr recognizes when the input hostname is an
 // IP address and returns it immediately to the caller.
 type resolverShortCircuitIPAddr struct {
-	model.Resolver
+	Resolver model.Resolver
 }
 
+var _ model.Resolver = &resolverShortCircuitIPAddr{}
+
 func (r *resolverShortCircuitIPAddr) LookupHost(ctx context.Context, hostname string) ([]string, error) {
 	if net.ParseIP(hostname) != nil {
 		return []string{hostname}, nil
@@ -222,6 +254,18 @@ func (r *resolverShortCircuitIPAddr) LookupHTTPS(ctx context.Context, hostname s
 	return r.Resolver.LookupHTTPS(ctx, hostname)
 }
 
+func (r *resolverShortCircuitIPAddr) Network() string {
+	return r.Resolver.Network()
+}
+
+func (r *resolverShortCircuitIPAddr) Address() string {
+	return r.Resolver.Address()
+}
+
+func (r *resolverShortCircuitIPAddr) CloseIdleConnections() {
+	r.Resolver.CloseIdleConnections()
+}
+
 // IsIPv6 returns true if the given candidate is a valid IP address
 // representation and such representation is IPv6.
 func IsIPv6(candidate string) (bool, error) {
@@ -271,7 +315,7 @@ func (r *nullResolver) LookupHTTPS(
 
 // resolverErrWrapper is a Resolver that knows about wrapping errors.
 type resolverErrWrapper struct {
-	model.Resolver
+	Resolver model.Resolver
 }
 
 var _ model.Resolver = &resolverErrWrapper{}
@@ -279,7 +323,7 @@ var _ model.Resolver = &resolverErrWrapper{}
 func (r *resolverErrWrapper) LookupHost(ctx context.Context, hostname string) ([]string, error) {
 	addrs, err := r.Resolver.LookupHost(ctx, hostname)
 	if err != nil {
-		return nil, NewErrWrapper(classifyResolverError, ResolveOperation, err)
+		return nil, newErrWrapper(classifyResolverError, ResolveOperation, err)
 	}
 	return addrs, nil
 }
@@ -288,7 +332,19 @@ func (r *resolverErrWrapper) LookupHTTPS(
 	ctx context.Context, domain string) (*model.HTTPSSvc, error) {
 	out, err := r.Resolver.LookupHTTPS(ctx, domain)
 	if err != nil {
-		return nil, NewErrWrapper(classifyResolverError, ResolveOperation, err)
+		return nil, newErrWrapper(classifyResolverError, ResolveOperation, err)
 	}
 	return out, nil
 }
+
+func (r *resolverErrWrapper) Network() string {
+	return r.Resolver.Network()
+}
+
+func (r *resolverErrWrapper) Address() string {
+	return r.Resolver.Address()
+}
+
+func (r *resolverErrWrapper) CloseIdleConnections() {
+	r.Resolver.CloseIdleConnections()
+}

internal/netxlite/resolver_test.go

@@ -400,6 +400,30 @@ func TestResolverIDNA(t *testing.T) {
 			}
 		})
 	})
+
+	t.Run("Network", func(t *testing.T) {
+		child := &mocks.Resolver{
+			MockNetwork: func() string {
+				return "x"
+			},
+		}
+		r := &resolverIDNA{child}
+		if r.Network() != "x" {
+			t.Fatal("invalid network")
+		}
+	})
+
+	t.Run("Address", func(t *testing.T) {
+		child := &mocks.Resolver{
+			MockAddress: func() string {
+				return "x"
+			},
+		}
+		r := &resolverIDNA{child}
+		if r.Address() != "x" {
+			t.Fatal("invalid address")
+		}
+	})
 }
 
 func TestResolverShortCircuitIPAddr(t *testing.T) {

internal/netxlite/serialresolver.go

@@ -1,7 +1,7 @@
 package netxlite
 
 //
-// Serial resolver implementation
+// Serial DNS resolver implementation
 //
 
 import (
@@ -20,7 +20,11 @@ import (
 //
 // You should probably use NewSerialResolver to create a new instance.
 //
-// Deprecated: please use ParallelResolver in new code.
+// Deprecated: please use ParallelResolver in new code. We cannot
+// remove this code as long as we use tracing for measuring.
+//
+// QUIRK: unlike the ParallelResolver, this resolver retries each
+// query three times for soft errors.
 type SerialResolver struct {
 	// Encoder is the MANDATORY encoder to use.
 	Encoder model.DNSEncoder
@@ -98,6 +102,8 @@ func (r *SerialResolver) LookupHTTPS(
 
 func (r *SerialResolver) lookupHostWithRetry(
 	ctx context.Context, hostname string, qtype uint16) ([]string, error) {
+	// QUIRK: retrying has been there since the beginning so we need to
+	// keep it as long as we're using tracing for measuring.
 	var errorslist []error
 	for i := 0; i < 3; i++ {
 		replies, err := r.lookupHostWithoutRetry(ctx, hostname, qtype)
@@ -116,9 +122,9 @@ func (r *SerialResolver) lookupHostWithRetry(
 		}
 		r.NumTimeouts.Add(1)
 	}
-	// bugfix: we MUST return one of the errors otherwise we confuse the
+	// QUIRK: we MUST return one of the errors otherwise we confuse the
 	// mechanism in errwrap that classifies the root cause operation, since
-	// it would not be able to find a child with a major operation error
+	// it would not be able to find a child with a major operation error.
 	return nil, errorslist[0]
 }
 

internal/netxlite/tls.go

@@ -1,5 +1,9 @@
 package netxlite
 
+//
+// TLS implementation
+//
+
 import (
 	"context"
 	"crypto/tls"
@@ -11,8 +15,12 @@ import (
 
 	oohttp "github.com/ooni/oohttp"
 	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/runtimex"
 )
 
+// TODO(bassosimone): check whether there's now equivalent functionality
+// inside the standard library allowing us to map numbers to names.
+
 var (
 	tlsVersionString = map[uint16]string{
 		tls.VersionTLS10: "TLSv1",
@@ -81,7 +89,8 @@ func NewDefaultCertPool() *x509.CertPool {
 	pool := x509.NewCertPool()
 	// Assumption: AppendCertsFromPEM cannot fail because we
 	// have a test in certify_test.go that guarantees that
-	pool.AppendCertsFromPEM([]byte(pemcerts))
+	ok := pool.AppendCertsFromPEM([]byte(pemcerts))
+	runtimex.PanicIfFalse(ok, "pool.AppendCertsFromPEM failed")
 	return pool
 }
 
@@ -201,8 +210,8 @@ var defaultTLSHandshaker = &tlsHandshakerConfigurable{}
 
 // tlsHandshakerLogger is a TLSHandshaker with logging.
 type tlsHandshakerLogger struct {
-	model.TLSHandshaker
+	TLSHandshaker model.TLSHandshaker
-	model.DebugLogger
+	DebugLogger   model.DebugLogger
 }
 
 var _ model.TLSHandshaker = &tlsHandshakerLogger{}
@@ -313,7 +322,7 @@ func NewSingleUseTLSDialer(conn TLSConn) model.TLSDialer {
 // tlsDialerSingleUseAdapter adapts dialerSingleUse to
 // be a TLSDialer type rather than a Dialer type.
 type tlsDialerSingleUseAdapter struct {
-	model.Dialer
+	Dialer model.Dialer
 }
 
 var _ model.TLSDialer = &tlsDialerSingleUseAdapter{}
@@ -323,9 +332,13 @@ func (d *tlsDialerSingleUseAdapter) DialTLSContext(ctx context.Context, network,
 	return d.Dialer.DialContext(ctx, network, address)
 }
 
+func (d *tlsDialerSingleUseAdapter) CloseIdleConnections() {
+	d.Dialer.CloseIdleConnections()
+}
+
 // tlsHandshakerErrWrapper wraps the returned error to be an OONI error
 type tlsHandshakerErrWrapper struct {
-	model.TLSHandshaker
+	TLSHandshaker model.TLSHandshaker
 }
 
 // Handshake implements TLSHandshaker.Handshake
@@ -334,7 +347,7 @@ func (h *tlsHandshakerErrWrapper) Handshake(
 ) (net.Conn, tls.ConnectionState, error) {
 	tlsconn, state, err := h.TLSHandshaker.Handshake(ctx, conn, config)
 	if err != nil {
-		return nil, tls.ConnectionState{}, NewErrWrapper(
+		return nil, tls.ConnectionState{}, newErrWrapper(
 			classifyTLSHandshakeError, TLSHandshakeOperation, err)
 	}
 	return tlsconn, state, nil

internal/netxlite/tls_test.go

@@ -487,6 +487,7 @@ func TestTLSDialer(t *testing.T) {
 func TestNewSingleUseTLSDialer(t *testing.T) {
 	conn := &mocks.TLSConn{}
 	d := NewSingleUseTLSDialer(conn)
+	defer d.CloseIdleConnections()
 	outconn, err := d.DialTLSContext(context.Background(), "", "")
 	if err != nil {
 		t.Fatal(err)

internal/netxlite/tproxy.go

@@ -1,5 +1,9 @@
 package netxlite
 
+//
+// Transparent proxy (for integration testing)
+//
+
 import (
 	"context"
 	"net"

internal/netxlite/utls.go

@@ -1,5 +1,9 @@
 package netxlite
 
+//
+// Code to use yawning/utls or refraction-networking/utls
+//
+
 import (
 	"context"
 	"crypto/tls"