Skip to content

Commit

Permalink
refactor(netxlite): use *Netx for the system dialer
Browse files Browse the repository at this point in the history
This diff is like 7224984
except that we are converting the system dialer constructors to
use *Netx rather than converting the system resolver.

While there, realize that DialerSystem could become private.

Part of ooni/probe#2531
  • Loading branch information
bassosimone committed Sep 12, 2023
1 parent 7224984 commit 37fd6b7
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 24 deletions.
23 changes: 15 additions & 8 deletions internal/netxlite/dialer.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,17 @@ func NewDialerWithStdlibResolver(dl model.DebugLogger) model.Dialer {
return NewDialerWithResolver(dl, reso)
}

// NewDialerWithResolver is equivalent to calling WrapDialer with
// the dialer argument being equal to &DialerSystem{}.
// NewDialerWithResolver is equivalent to calling WrapDialer with a dialer using the stdlib
// where the underlying dialer uses the [*Netx] UnderlyingNetwork for dialing.
func (netx *Netx) NewDialerWithResolver(dl model.DebugLogger, r model.Resolver, w ...model.DialerWrapper) model.Dialer {
return WrapDialer(dl, r, &dialerSystem{provider: netx.maybeCustomUnderlyingNetwork()}, w...)
}

// NewDialerWithResolver is equivalent to creating an empty [*Netx]
// and callings its NewDialerWithResolver method.
func NewDialerWithResolver(dl model.DebugLogger, r model.Resolver, w ...model.DialerWrapper) model.Dialer {
return WrapDialer(dl, r, &DialerSystem{}, w...)
netx := &Netx{Underlying: nil}
return netx.NewDialerWithResolver(dl, r, w...)
}

// WrapDialer wraps an existing Dialer to add extra functionality
Expand Down Expand Up @@ -142,24 +149,24 @@ func NewDialerWithoutResolver(dl model.DebugLogger, w ...model.DialerWrapper) mo
return NewDialerWithResolver(dl, &NullResolver{}, w...)
}

// DialerSystem is a model.Dialer that uses the stdlib's net.Dialer
// dialerSystem is a model.Dialer that uses the stdlib's net.Dialer
// to construct the new SimpleDialer used for dialing. This dialer has
// a fixed timeout for each connect operation equal to 15 seconds.
type DialerSystem struct {
type dialerSystem struct {
// provider is the OPTIONAL nil-safe [model.UnderlyingNetwork] provider.
provider *MaybeCustomUnderlyingNetwork
}

var _ model.Dialer = &DialerSystem{}
var _ model.Dialer = &dialerSystem{}

func (d *DialerSystem) DialContext(ctx context.Context, network, address string) (net.Conn, error) {
func (d *dialerSystem) DialContext(ctx context.Context, network, address string) (net.Conn, error) {
p := d.provider.Get()
ctx, cancel := context.WithTimeout(ctx, p.DialTimeout())
defer cancel()
return p.DialContext(ctx, network, address)
}

func (d *DialerSystem) CloseIdleConnections() {
func (d *dialerSystem) CloseIdleConnections() {
// nothing to do here
}

Expand Down
18 changes: 9 additions & 9 deletions internal/netxlite/dialer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ func TestNewDialerWithStdlibResolver(t *testing.T) {
t.Fatal("invalid logger")
}
errWrapper := logger.Dialer.(*dialerErrWrapper)
_ = errWrapper.Dialer.(*DialerSystem)
_ = errWrapper.Dialer.(*dialerSystem)
}

type extensionDialerFirst struct {
Expand Down Expand Up @@ -76,19 +76,19 @@ func TestNewDialer(t *testing.T) {
ext2 := logger.Dialer.(*extensionDialerSecond)
ext1 := ext2.Dialer.(*extensionDialerFirst)
errWrapper := ext1.Dialer.(*dialerErrWrapper)
_ = errWrapper.Dialer.(*DialerSystem)
_ = errWrapper.Dialer.(*dialerSystem)
})
}

func TestDialerSystem(t *testing.T) {
t.Run("CloseIdleConnections", func(t *testing.T) {
d := &DialerSystem{}
d := &dialerSystem{}
d.CloseIdleConnections() // to avoid missing coverage
})

t.Run("DialContext", func(t *testing.T) {
t.Run("with canceled context", func(t *testing.T) {
d := &DialerSystem{}
d := &dialerSystem{}
ctx, cancel := context.WithCancel(context.Background())
cancel() // immediately!
conn, err := d.DialContext(ctx, "tcp", "8.8.8.8:443")
Expand All @@ -108,7 +108,7 @@ func TestDialerSystem(t *testing.T) {
},
MockDialContext: defaultTp.DialContext,
}
d := &DialerSystem{provider: &MaybeCustomUnderlyingNetwork{tp}}
d := &dialerSystem{provider: &MaybeCustomUnderlyingNetwork{tp}}
ctx := context.Background()
start := time.Now()
conn, err := d.DialContext(ctx, "tcp", "dns.google:443")
Expand All @@ -134,7 +134,7 @@ func TestDialerSystem(t *testing.T) {
return nil, expected
},
}
d := &DialerSystem{provider: &MaybeCustomUnderlyingNetwork{proxy}}
d := &dialerSystem{provider: &MaybeCustomUnderlyingNetwork{proxy}}
conn, err := d.DialContext(context.Background(), "tcp", "dns.google:443")
if conn != nil {
t.Fatal("unexpected conn")
Expand All @@ -150,7 +150,7 @@ func TestDialerResolverWithTracing(t *testing.T) {
t.Run("DialContext", func(t *testing.T) {
t.Run("fails without a port", func(t *testing.T) {
d := &dialerResolverWithTracing{
Dialer: &DialerSystem{},
Dialer: &dialerSystem{},
Resolver: NewUnwrappedStdlibResolver(),
}
const missingPort = "ooni.nu"
Expand Down Expand Up @@ -497,7 +497,7 @@ func TestDialerResolverWithTracing(t *testing.T) {
t.Run("lookupHost", func(t *testing.T) {
t.Run("handles addresses correctly", func(t *testing.T) {
dialer := &dialerResolverWithTracing{
Dialer: &DialerSystem{},
Dialer: &dialerSystem{},
Resolver: &NullResolver{},
}
addrs, err := dialer.lookupHost(context.Background(), "1.1.1.1")
Expand All @@ -511,7 +511,7 @@ func TestDialerResolverWithTracing(t *testing.T) {

t.Run("fails correctly on lookup error", func(t *testing.T) {
dialer := &dialerResolverWithTracing{
Dialer: &DialerSystem{},
Dialer: &dialerSystem{},
Resolver: &NullResolver{},
}
ctx := context.Background()
Expand Down
6 changes: 0 additions & 6 deletions internal/netxlite/netx.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,6 @@ func (netx *Netx) maybeCustomUnderlyingNetwork() *MaybeCustomUnderlyingNetwork {
return &MaybeCustomUnderlyingNetwork{netx.Underlying}
}

// NewDialerWithResolver is like [netxlite.NewDialerWithResolver] but the constructed [model.Dialer]
// uses the [model.UnderlyingNetwork] configured inside the [Netx] structure.
func (n *Netx) NewDialerWithResolver(dl model.DebugLogger, r model.Resolver, w ...model.DialerWrapper) model.Dialer {
return WrapDialer(dl, r, &DialerSystem{provider: n.maybeCustomUnderlyingNetwork()}, w...)
}

// NewUDPListener is like [netxlite.NewUDPListener] but the constructed [model.UDPListener]
// uses the [model.UnderlyingNetwork] configured inside the [Netx] structure.
func (n *Netx) NewUDPListener() model.UDPListener {
Expand Down
2 changes: 1 addition & 1 deletion internal/netxlite/tls_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -711,7 +711,7 @@ func TestTLSDialer(t *testing.T) {
t.Run("failure dialing", func(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
cancel() // immediately fail
dialer := tlsDialer{Dialer: &DialerSystem{}}
dialer := tlsDialer{Dialer: &dialerSystem{}}
conn, err := dialer.DialTLSContext(ctx, "tcp", "www.google.com:443")
if err == nil || !strings.HasSuffix(err.Error(), "operation was canceled") {
t.Fatal("not the error we expected", err)
Expand Down

0 comments on commit 37fd6b7

Please sign in to comment.