Apply suggestions from code review

internal/ptx/obfs4.go

@@ -84,7 +84,7 @@ func (d *OBFS4Dialer) newCancellableDialer() (*obfs4CancellableDialer, error) {
 func (d *OBFS4Dialer) newFactory() base.ClientFactory {
 	o4f := &obfs4.Transport{}
 	cf, err := o4f.ClientFactory(filepath.Join(d.DataDir, "obfs4"))
-	// the source code for this transport always returns nil
+	// the source code for this transport always returns a nil error
 	runtimex.PanicOnError(err, "unexpected o4f.ClientFactory failure")
 	return cf
 }
@@ -111,7 +111,7 @@ func (d *OBFS4Dialer) underlyingDialer() UnderlyingDialer {
 type obfs4CancellableDialer struct {
 	// done is a channel that will be closed when done. In normal
 	// usage you don't want to await for this signal. But it's useful
-	// for testing to know that the background goroutine joned.
+	// for testing to know that the background goroutine joined.
 	done chan interface{}
 
 	// factory is the factory for obfs4.

internal/ptx/obfs4_test.go

@@ -13,7 +13,7 @@ import (
 
 func TestOBFS4DialerWorks(t *testing.T) {
 	// This test is 0.3 seconds in my machine, so it's ~fine
-	// to run it when we're in short mode
+	// to run it even when we're in short mode
 	o4d := DefaultTestingOBFS4Bridge()
 	conn, err := o4d.DialContext(context.Background())
 	if err != nil {

internal/ptx/ptx.go

@@ -64,7 +64,7 @@ type PTDialer interface {
 }
 
 // Listener is a generic pluggable transports listener. Make sure
-// you fill the mandatory fields before using. Do not modify public
+// you fill the mandatory fields before using it. Do not modify public
 // fields after you called Start, since this causes data races.
 type Listener struct {
 	// PTDialer is the MANDATORY pluggable transports dialer
@@ -80,7 +80,7 @@ type Listener struct {
 	// mu provides mutual exclusion for accessing internals.
 	mu sync.Mutex
 
-	// cancel allows to stop the forwarders.
+	// cancel allows stopping the forwarders.
 	cancel context.CancelFunc
 
 	// laddr is the listen address.
@@ -149,7 +149,7 @@ func (lst *Listener) handleSocksConn(ctx context.Context, socksConn ptxSocksConn
 		lst.logger().Warnf("ptx: ContextDialer.DialContext error: %s", err)
 		return err // used for testing
 	}
-	lst.forwardWithContext(ctx, socksConn, ptConn)
+	lst.forwardWithContext(ctx, socksConn, ptConn) // transfer ownership
 	return nil // used for testing
 }
 
@@ -170,7 +170,7 @@ type ptxSocksConn interface {
 	// net.Conn is the embedded interface.
 	net.Conn
 
-	// Grants access to a specific IP address.
+	// Grant grants access to a specific IP address.
 	Grant(addr *net.TCPAddr) error
 }
 
@@ -184,7 +184,7 @@ func (lst *Listener) acceptLoop(ctx context.Context, ln ptxSocksListener) {
 				continue
 			}
 			lst.logger().Warnf("ptx: socks accept error: %s", err)
-			break
+			return
 		}
 		go lst.handleSocksConn(ctx, conn)
 	}
@@ -193,7 +193,7 @@ func (lst *Listener) acceptLoop(ctx context.Context, ln ptxSocksListener) {
 // Addr returns the listening address. This function should not
 // be called after you have called the Stop method or before the
 // Start method has successfully returned. When invoked in such
-// conditions, this function will return nil. Otherwise, it will
+// conditions, this function may return nil. Otherwise, it will
 // return the valid net.Addr where we are listening.
 func (lst *Listener) Addr() net.Addr {
 	return lst.laddr
@@ -250,7 +250,9 @@ func (la *ptxSocksListenerAdapter) AcceptSocks() (ptxSocksConn, error) {
 	return la.SocksListener.AcceptSocks()
 }
 
-// torCmdLine prints the command line for testing this listener.
+// torCmdLine prints the command line for testing this listener. This method is here to
+// facilitate debugging with `ptxclient`, so there is no need to be too precise with arguments
+// quoting. Remember to improve upon this aspect if you plan on using it beyond testing.
 func (lst *Listener) torCmdLine() string {
 	return strings.Join([]string{
 		"tor",

internal/ptx/ptx_test.go

@@ -103,7 +103,7 @@ type mockableSocksConn struct {
 	MockGrant func(addr *net.TCPAddr) error
 }
 
-// Grants access to a specific IP address.
+// Grant grants access to a specific IP address.
 func (c *mockableSocksConn) Grant(addr *net.TCPAddr) error {
 	return c.MockGrant(addr)
 }
@@ -180,15 +180,13 @@ func TestListenerForwardWithContextWithContextCancellation(t *testing.T) {
 	lst := &Listener{}
 	left, right := net.Pipe()
 	go lst.forwardWithContext(ctx, left, right)
-	time.Sleep(100 * time.Millisecond)
 	cancel()
 }
 
 func TestListenerForwardWithNaturalTermination(t *testing.T) {
 	lst := &Listener{}
 	left, right := net.Pipe()
 	go lst.forwardWithContext(context.Background(), left, right)
-	time.Sleep(100 * time.Millisecond)
 	right.Close()
 }
 

internal/ptx/snowflake.go

@@ -58,7 +58,7 @@ func (d *SnowflakeDialer) dialContext(
 	}
 	connch, errch := make(chan net.Conn), make(chan error, 1)
 	go func() {
-		defer close(done) // allow tests to synchronize with our end
+		defer close(done) // allow tests to synchronize with this goroutine's exit
 		conn, err := txp.Dial()
 		if err != nil {
 			errch <- err // buffered channel
@@ -90,8 +90,8 @@ func (d *SnowflakeDialer) newSnowflakeClient(brokerURL string, frontDomain strin
 			keepLocalAddresses, maxSnowflakes)
 	}
 	return sflib.NewSnowflakeClient(
-		d.brokerURL(), d.frontDomain(), d.iceAddresses(),
-		false, d.maxSnowflakes())
+		brokerURL, frontDomain, iceAddresses,
+		keepLocalAddresses, maxSnowflakes)
 }
 
 // brokerURL returns a suitable broker URL.