Skip to content

Commit

Permalink
refactor(netxlite): use *Netx for the system dialer (#1249)
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 authored Sep 12, 2023
1 parent 7224984 commit 07a048c
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 25 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
// the [*Netx] UnderlyingNetwork for dialing new connections.
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 calling 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/resolvercore.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func (netx *Netx) NewStdlibResolver(logger model.DebugLogger) model.Resolver {
}

// NewStdlibResolver is equivalent to creating an empty [*Netx]
// and callings its NewStdlibResolver method.
// and calling its NewStdlibResolver method.
func NewStdlibResolver(logger model.DebugLogger) model.Resolver {
netx := &Netx{Underlying: nil}
return netx.NewStdlibResolver(logger)
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 07a048c

Please sign in to comment.