feat(enginenetx): make sure HTTPSDialer closes all connections

internal/enginenetx/httpsdialer_internal_test.go

@@ -4,6 +4,7 @@ import (
  "context"
  "crypto/tls"
  "errors"
+ "fmt"
  "sync"
  "sync/atomic"
  "testing"
@@ -25,31 +26,28 @@ func TestHTTPSDialerTacticsEmitter(t *testing.T) {
  wg: &sync.WaitGroup{},
  }
 
- tactics := []HTTPSDialerTactic{
- &httpsDialerNullTactic{
- Address: "10.0.0.1",
+ var tactics []HTTPSDialerTactic
+ for idx := 0; idx < 255; idx++ {
+ tactics = append(tactics, &httpsDialerNullTactic{
+ Address: fmt.Sprintf("10.0.0.%d", idx),
  Delay: 0,
  Domain: "www.example.com",
- },
- &httpsDialerNullTactic{
- Address: "10.0.0.2",
- Delay: 0,
- Domain: "www.example.com",
- },
+ })
  }
 
  ctx, cancel := context.WithCancel(context.Background())
  cancel() // we want the tested function to run with a canceled context
 
  out := hd.tacticsEmitter(ctx, tactics...)
 
- var count int
  for range out {
- count++
- }
-
- if count != 0 {
- t.Fatal("nothing should have been emitted here")
+ // Here we do nothing!
+ //
+ // Ideally, we would like to count and assert that we have
+ // got no tactic from the channel but the selection of ready
+ // channels is nondeterministic, so we cannot really be
+ // asserting that. This leaves us with asking the question
+ // of what we should be asserting here?
  }
  })
 }

internal/enginenetx/httpsdialer_test.go

@@ -11,6 +11,7 @@ import (
  "github.com/ooni/probe-cli/v3/internal/model"
  "github.com/ooni/probe-cli/v3/internal/netemx"
  "github.com/ooni/probe-cli/v3/internal/netxlite"
+ "github.com/ooni/probe-cli/v3/internal/testingx"
 )
 
 // Flags controlling when [httpsDialerPolicyCancelingContext] cancels the context
@@ -355,70 +356,81 @@ func TestHTTPSDialerWAI(t *testing.T) {
  t.Skip("skip test in short mode")
  }
 
- // create the QA environment
- env := netemx.MustNewScenario(tc.scenario)
- defer env.Close()
+ // track all the connections so we can check whether we close them all
+ cv := &testingx.CloseVerify{}
 
- // possibly add specific DPI rules
- tc.configureDPI(env.DPIEngine())
+ func() {
+ // create the QA environment
+ env := netemx.MustNewScenario(tc.scenario)
+ defer env.Close()
 
- // create the proper underlying network
- unet := &netxlite.NetemUnderlyingNetworkAdapter{UNet: env.ClientStack}
+  // possibly add specific DPI rules
+  tc.configureDPI(env.DPIEngine())
 
- // create the network proper
- netx := &netxlite.Netx{Underlying: unet}
+ // create the proper underlying network and wrap it such that
+ // we track whether we close all the connections
+ unet := cv.WrapUnderlyingNetwork(&netxlite.NetemUnderlyingNetworkAdapter{UNet: env.ClientStack})
 
- // create the getaddrinfo resolver
- resolver := netx.NewStdlibResolver(log.Log)
+  // create the network proper
+  netx := &netxlite.Netx{Underlying: unet}
 
- // create the TLS dialer
- dialer := enginenetx.NewHTTPSDialer(
- log.Log,
- tc.policy,
- resolver,
- unet,
- )
- defer dialer.CloseIdleConnections()
+ // create the getaddrinfo resolver
+ resolver := netx.NewStdlibResolver(log.Log)
 
- // configure cancellable context--some tests are going to use cancel
- ctx, cancel := context.WithCancel(context.Background())
- defer cancel()
+ // create the TLS dialer
+ dialer := enginenetx.NewHTTPSDialer(
+ log.Log,
+ tc.policy,
+ resolver,
+ unet,
+ )
+ defer dialer.CloseIdleConnections()
 
- // Possibly tell the httpsDialerPolicyCancelingContext about the cancel func
- // depending on which flags have been configured.
- if p, ok := tc.policy.(*httpsDialerPolicyCancelingContext); ok {
- p.cancel = cancel
- }
+ // configure cancellable context--some tests are going to use cancel
+ ctx, cancel := context.WithCancel(context.Background())
+ defer cancel()
+
+ // Possibly tell the httpsDialerPolicyCancelingContext about the cancel func
+ // depending on which flags have been configured.
+ if p, ok := tc.policy.(*httpsDialerPolicyCancelingContext); ok {
+ p.cancel = cancel
+ }
+
+ // dial the TLS connection
+ tlsConn, err := dialer.DialTLSContext(ctx, "tcp", tc.endpoint)
 
- // dial the TLS connection
- tlsConn, err := dialer.DialTLSContext(ctx, "tcp", tc.endpoint)
+ t.Logf("%+v %+v", tlsConn, err)
 
- t.Logf("%+v %+v", tlsConn, err)
+ // make sure the error is the one we expected
+ switch {
+ case err != nil && tc.expectErr == "":
+ t.Fatal("expected", tc.expectErr, "got", err)
 
- // make sure the error is the one we expected
- switch {
- case err != nil && tc.expectErr == "":
- t.Fatal("expected", tc.expectErr, "got", err)
+ case err == nil && tc.expectErr != "":
+ t.Fatal("expected", tc.expectErr, "got", err)
 
- case err == nil && tc.expectErr != "":
- t.Fatal("expected", tc.expectErr, "got", err)
+ case err != nil && tc.expectErr != "":
+ if diff := cmp.Diff(tc.expectErr, err.Error()); diff != "" {
+ t.Fatal(diff)
+ }
 
- case err != nil && tc.expectErr != "":
- if diff := cmp.Diff(tc.expectErr, err.Error()); diff != "" {
- t.Fatal(diff)
+ case err == nil && tc.expectErr == "":
+ // all good
  }
 
- case err == nil && tc.expectErr == "":
- // all good
- }
+ // make sure we close the conn
+ if tlsConn != nil {
+ defer tlsConn.Close()
+ }
 
- // make sure we close the conn
- if tlsConn != nil {
- defer tlsConn.Close()
- }
+ // wait for background connections to join
+ dialer.WaitGroup().Wait()
+ }()
 
- // wait for background connections to join
- dialer.WaitGroup().Wait()
+ // now verify that we have closed all the connections
+ if err := cv.CheckForOpenConns(); err != nil {
+ t.Fatal(err)
+ }
  })
  }
 }