Add WithDialer option and rewrite With* to auto-inject dialer by myleshorton · Pull Request #29 · getlantern/kindling

myleshorton · 2026-02-22T13:03:32Z

Summary

Add exported DialContextFunc type and WithDialer option that automatically flows to all transports
Add Close() to Kindling interface for resource cleanup
Rewrite WithDomainFronting to accept fronted.Option params and construct the instance internally
Rewrite WithDNSTunnel to accept dnstt.Option params and construct the instance internally
Rewrite WithAMPCache to accept amp.Config + amp.Option params and construct the instance internally
Update smart dialer to use injected dialer via FuncStreamDialer + streamConnAdapter

Depends on getlantern/fronted#67 and getlantern/dnstt#12.

Test plan

go build ./... compiles cleanly
go vet ./... passes
TestKindling, TestNewKindling, TestKindling_ReplaceTransport, TestWithDialer all pass
Verify end-to-end with fronted transport
Verify end-to-end with custom dialer injected

🤖 Generated with Claude Code

Introduce a single WithDialer option at the kindling level that automatically flows to all transports (fronted, dnstt, amp, smart). Key changes: - Add exported DialContextFunc type and WithDialer option - Add Close() to Kindling interface for resource cleanup - Rewrite WithDomainFronting to accept fronted.Option params - Rewrite WithDNSTunnel to accept dnstt.Option params - Rewrite WithAMPCache to accept amp.Config + amp.Option params - Update smart dialer to use injected dialer via FuncStreamDialer - Add streamConnAdapter and closerFunc helper types - Update tests for new API signatures The With* functions now construct transport instances internally and prepend the dialer option, so callers no longer need to create transport instances themselves. Requires getlantern/fronted#67 and getlantern/dnstt#12. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Update fronted to latest commit which introduces the named DialFunc type. Add explicit type conversion for the dialer passed to fronted.WithDialer. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This pull request refactors the kindling library's transport configuration to enable automatic dialer injection across all transports. The changes introduce a canonical DialContextFunc type and a WithDialer option that flows to all transports, while rewriting the transport setup functions (WithDomainFronting, WithDNSTunnel, WithAMPCache) to accept options and construct instances internally.

Changes:

Introduced DialContextFunc type and WithDialer option for custom dialer injection
Added Close() method to Kindling interface for proper resource cleanup
Rewrote WithDomainFronting, WithDNSTunnel, and WithAMPCache to accept options and auto-inject the dialer
Updated smart dialer integration to use injected dialer via FuncStreamDialer and streamConnAdapter
Updated dependencies to newer versions of dnstt and fronted libraries

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 9 comments.

File	Description
kindling.go	Core implementation: added `DialContextFunc` type, `Close()` method, dialer injection logic, and refactored transport setup functions
kindling_test.go	Updated tests to use new `WithDomainFronting` API and added `TestWithDialer` test; added `Close()` calls for cleanup
go.mod	Updated `dnstt` and `fronted` dependency versions
go.sum	Updated checksums for new dependency versions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-22T14:16:45Z

kindling.go

+// streamConnAdapter wraps a net.Conn as a transport.StreamConn by adding a no-op CloseWrite/CloseRead.
+type streamConnAdapter struct {
+	net.Conn
 }

+func (s *streamConnAdapter) CloseWrite() error { return nil }
+func (s *streamConnAdapter) CloseRead() error  { return nil }
+


The streamConnAdapter implements CloseWrite and CloseRead as no-ops that always return nil. This could hide errors during connection shutdown and may not properly signal the remote end that writing is complete. If the underlying transport.StreamConn interface requires proper half-close semantics (like TCP's shutdown(SHUT_WR)), this implementation could lead to incorrect behavior. Consider checking if the underlying net.Conn supports these operations (e.g., via type assertion to *net.TCPConn) and calling the actual shutdown methods when available.

Suggested change

// streamConnAdapter wraps a net.Conn as a transport.StreamConn by adding a no-op CloseWrite/CloseRead.

type streamConnAdapter struct {

net.Conn

}

func (s *streamConnAdapter) CloseWrite() error { return nil }

func (s *streamConnAdapter) CloseRead() error { return nil }

// streamConnAdapter wraps a net.Conn as a transport.StreamConn by adding CloseWrite/CloseRead methods.

type streamConnAdapter struct {

net.Conn

}

func (s *streamConnAdapter) CloseWrite() error {

if cw, ok := s.Conn.(interface{ CloseWrite() error }); ok {

return cw.CloseWrite()

}

if tcp, ok := s.Conn.(*net.TCPConn); ok {

return tcp.CloseWrite()

}

return nil

}

func (s *streamConnAdapter) CloseRead() error {

if cr, ok := s.Conn.(interface{ CloseRead() error }); ok {

return cr.CloseRead()

}

if tcp, ok := s.Conn.(*net.TCPConn); ok {

return tcp.CloseRead()

}

return nil

}

Copilot · 2026-02-22T14:16:46Z

kindling.go

+func (k *kindling) Close() error {
+	var errs []error
+	for _, c := range k.closers {
+		if err := c.Close(); err != nil {
+			errs = append(errs, err)
+		}
+	}
+	if len(errs) > 0 {
+		return fmt.Errorf("errors closing kindling resources: %v", errs)
+	}
+	return nil
+}


The closers slice is not protected by a mutex, which could lead to race conditions if Close() is called concurrently with options being applied or if multiple goroutines call Close() simultaneously. The transports slice has a mutex (roundTripperGeneratorsMutex) protecting it, but closers does not. Consider adding synchronization to protect the closers slice and the Close() method, or document that Close() must not be called concurrently.

Copilot · 2026-02-22T14:16:46Z

kindling_test.go

+func TestWithDialer(t *testing.T) {
+	t.Parallel()
+	var called atomic.Bool
+	customDialer := func(ctx context.Context, network, addr string) (net.Conn, error) {
+		called.Store(true)
+		return (&net.Dialer{}).DialContext(ctx, network, addr)
+	}
+	k := NewKindling("test-app",
+		WithDialer(customDialer),
+	)
+	defer k.Close()
+	if k == nil {
+		t.Fatal("NewKindling() = nil; want non-nil Kindling")
+	}
+	// Verify the dialer was stored by checking the internal struct.
+	ki := k.(*kindling)
+	if ki.dialContext == nil {
+		t.Fatal("dialContext should not be nil after WithDialer")
+	}
+}


The TestWithDialer test doesn't actually verify that the custom dialer is called. The 'called' atomic.Bool is set up but never checked after operations that should trigger the dialer. Consider making a test request using k.NewHTTPClient() to verify the dialer is actually used, or at minimum check that called.Load() returns true at the end of the test if the dialer should have been invoked during initialization.

Copilot · 2026-02-22T14:16:46Z

kindling.go

+		d, err := dnstt.NewDNSTT(allOpts...)
+		if err != nil {
+			log.Error("Failed to create DNSTT instance", "error", err)
+			return
+		}


When dnstt.NewDNSTT fails, the error is logged but the function returns silently without any indication that the transport wasn't added. This could lead to a Kindling instance that appears to be configured but is missing a transport. Consider whether this silent failure is intentional, or if the error should be propagated or handled differently. For consistency, WithAMPCache has similar error handling at lines 201-204.

Copilot · 2026-02-22T14:16:46Z

kindling.go

+		ampDialer := func(network, addr string) (net.Conn, error) {
+			return k.dialContext(context.Background(), network, addr)
+		}


In the ampDialer adapter function, context.Background() is hardcoded instead of using the context that would be passed by the AMP client. This means any timeout, cancellation, or deadline in the AMP client's context will be ignored during dialing. Consider if the amp.WithDialer signature can accept a DialContextFunc directly, or if the adapter needs to be redesigned to properly propagate context from the caller.

Copilot · 2026-02-22T14:16:46Z

kindling.go

+		ctx, cancel := context.WithCancel(context.Background())
+		k.closers = append(k.closers, closerFunc(func() error { cancel(); return nil }))
+
+		c, err := amp.NewClientWithConfig(ctx, cfg, allOpts...)
+		if err != nil {
+			cancel()
+			log.Error("Failed to create AMP client", "error", err)
+			return
+		}


In WithAMPCache, a context with cancel is created for the AMP client, and the cancel function is added to closers. However, if amp.NewClientWithConfig returns an error, the cancel is called on line 202, but the closerFunc wrapping it was already added to k.closers on line 198. This means Close() will call cancel() again after it's already been called, which while safe (cancel is idempotent), represents incorrect cleanup ordering. The closerFunc should only be added to k.closers after successful client creation. Move line 198 to after the error check on line 205.

Copilot · 2026-02-22T14:16:46Z

kindling.go

+func WithAMPCache(cfg amp.Config, opts ...amp.Option) Option {
+	return newOption(func(k *kindling) {
+		log.Info("Setting AMP cache")
+		// Adapt DialContextFunc to amp's dialFunc (func(network, addr string) (net.Conn, error))
+		ampDialer := func(network, addr string) (net.Conn, error) {
+			return k.dialContext(context.Background(), network, addr)
+		}
+		allOpts := make([]amp.Option, 0, len(opts)+1)
+		allOpts = append(allOpts, amp.WithDialer(ampDialer))
+		allOpts = append(allOpts, opts...)
+
+		ctx, cancel := context.WithCancel(context.Background())
+		k.closers = append(k.closers, closerFunc(func() error { cancel(); return nil }))
+
+		c, err := amp.NewClientWithConfig(ctx, cfg, allOpts...)
+		if err != nil {
+			cancel()
+			log.Error("Failed to create AMP client", "error", err)
+			return
+		}
+		k.transports = append(k.transports, newTransport("amp", 6000, false, func(ctx context.Context, addr string) (http.RoundTripper, error) {
+			return c.RoundTripper()
+		}))
+	})


The signature of WithAMPCache has changed from accepting an amp.Client to accepting amp.Config as a required first parameter plus optional amp.Option parameters. This is a breaking API change that will require all existing callers to be updated. While this change aligns with the pattern used in WithDomainFronting and WithDNSTunnel (accepting options instead of pre-constructed instances), existing code that calls WithAMPCache(client) will fail to compile. Ensure this breaking change is documented and that all call sites in dependent projects are updated.

Copilot · 2026-02-22T14:16:47Z

kindling.go

+func WithDomainFronting(opts ...fronted.Option) Option {
+	return newOption(func(k *kindling) {
+		log.Info("Setting domain fronting")
+		// Prepend our dialer so the caller's options can override if needed.
+		allOpts := make([]fronted.Option, 0, len(opts)+1)
+		allOpts = append(allOpts, fronted.WithDialer(fronted.DialFunc(k.dialContext)))
+		allOpts = append(allOpts, opts...)
+		f := fronted.NewFronted(allOpts...)
+		k.closers = append(k.closers, closerFunc(func() error { f.Close(); return nil }))
+		k.transports = append(k.transports, newTransport("fronted", 0, true, func(ctx context.Context, addr string) (http.RoundTripper, error) {
+			return f.NewConnectedRoundTripper(ctx, addr)
+		}))
+	})
 }


The signature of WithDomainFronting has changed from accepting a fronted.Fronted instance to accepting fronted.Option parameters. This is a breaking API change that will require all existing callers to be updated. While this change enables better integration with the dialer injection pattern, existing code that calls WithDomainFronting(frontedInstance) will fail to compile. Ensure this breaking change is documented and that all call sites in dependent projects are updated.

Copilot · 2026-02-22T14:16:47Z

kindling.go

+func WithDNSTunnel(opts ...dnstt.Option) Option {
+	return newOption(func(k *kindling) {
+		log.Info("Setting DNS tunnel")
+		// Prepend our dialer so the caller's options can override if needed.
+		allOpts := make([]dnstt.Option, 0, len(opts)+1)
+		allOpts = append(allOpts, dnstt.WithDialer(k.dialContext))
+		allOpts = append(allOpts, opts...)
+		d, err := dnstt.NewDNSTT(allOpts...)
+		if err != nil {
+			log.Error("Failed to create DNSTT instance", "error", err)
+			return
+		}
+		k.closers = append(k.closers, d)
+		k.transports = append(k.transports, newTransport("dnstt", 0, true, func(ctx context.Context, addr string) (http.RoundTripper, error) {
+			return d.NewRoundTripper(ctx, addr)
+		}))
+	})
+}


The signature of WithDNSTunnel has changed from accepting a dnstt.DNSTT instance to accepting dnstt.Option parameters. This is a breaking API change that will require all existing callers to be updated. While this change enables better integration with the dialer injection pattern, existing code that calls WithDNSTunnel(dnsttInstance) will fail to compile. Ensure this breaking change is documented and that all call sites in dependent projects are updated.

- streamConnAdapter: delegate CloseWrite/CloseRead to underlying conn when it supports the interface, instead of no-ops - Add mutex to protect closers slice for concurrent Close() safety - Fix WithAMPCache: only add cancel closer after successful client creation - Add comment explaining context.Background() limitation in amp adapter - Improve TestWithDialer to verify custom dialer is actually called - Add TestWithDialerDefault for default dialer behavior - Document breaking API changes in WithDomainFronting, WithDNSTunnel, WithAMPCache doc comments - Document silent failure behavior in WithDNSTunnel and WithAMPCache Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

myleshorton and others added 3 commits February 22, 2026 06:02

Update dnstt dependency to latest add-with-dialer commit

32261d5

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Update fronted dependency and fix DialFunc type conversion

891684b

Update fronted to latest commit which introduces the named DialFunc type. Add explicit type conversion for the dialer passed to fronted.WithDialer. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

myleshorton requested a review from Copilot February 22, 2026 14:12

Copilot started reviewing on behalf of myleshorton February 22, 2026 14:12 View session

Copilot AI reviewed Feb 22, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WithDialer option and rewrite With* to auto-inject dialer#29

Add WithDialer option and rewrite With* to auto-inject dialer#29
myleshorton wants to merge 4 commits intomainfrom
add-dialer-injection

myleshorton commented Feb 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Copilot AI Feb 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

myleshorton commented Feb 22, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants