doc(netx): reference issue mentioning future improvements

See ooni/probe#2121 (comment)
ooni · Jun 6, 2022 · 5c071e9 · 5c071e9
1 parent 2502a23
commit 5c071e9
Show file tree

Hide file tree

Showing 4 changed files with 20 additions and 8 deletions.
diff --git a/internal/engine/netx/dialer.go b/internal/engine/netx/dialer.go
@@ -13,6 +13,7 @@ import (
 // NewDialer creates a new Dialer from the specified config.
 func NewDialer(config Config) model.Dialer {
 	if config.FullResolver == nil {
+		// TODO(https://github.com/ooni/probe/issues/2121#issuecomment-1147424810)
 		config.FullResolver = NewResolver(config)
 	}
 	logger := model.ValidLoggerOrDefault(config.Logger)

diff --git a/internal/engine/netx/dnstransport.go b/internal/engine/netx/dnstransport.go
@@ -6,6 +6,7 @@ package netx
 // TODO(bassosimone): this code should be refactored to return
 // a DNSTransport rather than a model.Resolver. With this in mind,
 // I've named this file dnstransport.go.
+// TODO(https://github.com/ooni/probe/issues/2121#issuecomment-1147424810)
 //
 
 import (
@@ -45,6 +46,8 @@ func NewDNSClient(config Config, URL string) (model.Resolver, error) {
 // with the option to override the default Hostname and SNI.
 func NewDNSClientWithOverrides(config Config, URL, hostOverride, SNIOverride,
 	TLSVersion string) (model.Resolver, error) {
+	// We should split this function in smaller and testable units
+	// TODO(https://github.com/ooni/probe/issues/2121#issuecomment-1147424810)
 	switch URL {
 	case "doh://powerdns":
 		URL = "https://doh.powerdns.org/"
@@ -132,6 +135,10 @@ func makeValidEndpoint(URL *url.URL) (string, error) {
 	if _, _, err := net.SplitHostPort(URL.Host); err == nil {
 		return URL.Host, nil
 	}
+
+	// Here we should add a test case for when the host is empty
+	// TODO(https://github.com/ooni/probe/issues/2121#issuecomment-1147424810)
+
 	// The second step is to assume that appending the default port
 	// to a host parsed by url.Parse should be giving us a valid
 	// endpoint. The possibilities in fact are:

diff --git a/internal/engine/netx/http.go b/internal/engine/netx/http.go
@@ -14,12 +14,15 @@ import (
 // NewHTTPTransport creates a new HTTPRoundTripper from the given Config.
 func NewHTTPTransport(config Config) model.HTTPTransport {
 	if config.Dialer == nil {
+		// TODO(https://github.com/ooni/probe/issues/2121#issuecomment-1147424810)
 		config.Dialer = NewDialer(config)
 	}
 	if config.TLSDialer == nil {
+		// TODO(https://github.com/ooni/probe/issues/2121#issuecomment-1147424810)
 		config.TLSDialer = NewTLSDialer(config)
 	}
 	if config.QUICDialer == nil {
+		// TODO(https://github.com/ooni/probe/issues/2121#issuecomment-1147424810)
 		config.QUICDialer = NewQUICDialer(config)
 	}
 	tInfo := allTransportsInfo[config.HTTP3Enabled]
@@ -30,7 +33,8 @@ func NewHTTPTransport(config Config) model.HTTPTransport {
 		TLSDialer:  config.TLSDialer,
 		TLSConfig:  config.TLSConfig,
 	})
-	// TODO(bassosimone): I am not super convinced by this code because it
+	// TODO(https://github.com/ooni/probe/issues/2121#issuecomment-1147424810): I am
+	// not super convinced by this code because it
 	// seems we're currently counting bytes twice in some cases. I think we
 	// should review how we're counting bytes and using netx currently.
 	txp = config.ByteCounter.MaybeWrapHTTPTransport(txp)                 // WAI with ByteCounter == nil
@@ -46,7 +50,7 @@ type httpTransportInfo struct {
 
 var allTransportsInfo = map[bool]httpTransportInfo{
 	false: {
-		Factory:       newSystemTransport,
+		Factory:       newHTTPTransport,
 		TransportName: "tcp",
 	},
 	true: {
@@ -56,6 +60,8 @@ var allTransportsInfo = map[bool]httpTransportInfo{
 }
 
 // httpTransportConfig contains configuration for constructing an HTTPTransport.
+//
+// All the fields in this structure MUST be initialized.
 type httpTransportConfig struct {
 	Dialer     model.Dialer
 	Logger     model.Logger
@@ -66,13 +72,10 @@ type httpTransportConfig struct {
 
 // newHTTP3Transport creates a new HTTP3Transport instance.
 func newHTTP3Transport(config httpTransportConfig) model.HTTPTransport {
-	// Rationale for using NoLogger here: previously this code did
-	// not use a logger as well, so it's fine to keep it as is.
 	return netxlite.NewHTTP3Transport(config.Logger, config.QUICDialer, config.TLSConfig)
 }
 
-// newSystemTransport creates a new "system" HTTP transport. That is a transport
-// using the Go standard library with custom dialer and TLS dialer.
-func newSystemTransport(config httpTransportConfig) model.HTTPTransport {
+// newHTTPTransport creates a new "system" HTTP transport.
+func newHTTPTransport(config httpTransportConfig) model.HTTPTransport {
 	return netxlite.NewHTTPTransport(config.Logger, config.Dialer, config.TLSDialer)
 }
diff --git a/internal/engine/netx/quic.go b/internal/engine/netx/quic.go
@@ -14,7 +14,8 @@ func NewQUICDialer(config Config) model.QUICDialer {
 	if config.FullResolver == nil {
 		config.FullResolver = NewResolver(config)
 	}
-	// TODO(bassosimone): we should count the bytes consumed by this QUIC dialer
+	// TODO(https://github.com/ooni/probe/issues/2121#issuecomment-1147424810): we
+	// should count the bytes consumed by this QUIC dialer
 	ql := config.ReadWriteSaver.WrapQUICListener(netxlite.NewQUICListener())
 	logger := model.ValidLoggerOrDefault(config.Logger)
 	return netxlite.NewQUICDialerWithResolver(ql, logger, config.FullResolver, config.Saver)