Skip to content

Conversation

myleshorton
Copy link
Contributor

This pull request introduces a new useProxyless function to enable or disable proxyless dialing dynamically across the application. The changes primarily involve replacing the previous ProxyAll functionality with UseProxyless and updating the relevant code to reflect this new approach. Below are the key changes grouped by theme:

Client Updates:

  • Added a useProxyless field to the Client struct in client/client.go, allowing the client to determine whether to use proxyless dialing dynamically.
  • Updated the NewClient constructor to accept the useProxyless function and initialize it in the Client struct. [1] [2]
  • Modified the initDialers method to pass the useProxyless function to the dialer options instead of the previous proxyAll function.

Dialer Updates:

  • Replaced the ProxyAll field in the Options struct with UseProxyless, and updated the corresponding comments to reflect the new functionality in dialer/dialer.go.
  • Updated the Clone method of the Options struct to include the new UseProxyless field.
  • Modified the newParallelPreferProxyless function in dialer/parallel_dialer.go to check both common.SupportsProxyless() and the UseProxyless function before falling back to the default dialer.

Flashlight Updates:

  • Added a useProxyless field to the Flashlight struct in flashlight.go to propagate the proxyless functionality.
  • Updated the New function in flashlight.go to pass the useProxyless function to the client during initialization.

Option Enhancements:

  • Introduced a new WithUseProxyless option in option.go to allow external configuration of the useProxyless function for the Flashlight client.

@myleshorton myleshorton requested a review from Copilot July 27, 2025 17:55
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request transitions from a proxyAll approach to a more explicit useProxyless configuration mechanism. The change allows dynamic control over whether proxyless dialing should be enabled, providing clearer semantics around when to use direct connections versus proxy connections.

Key changes:

  • Replaces ProxyAll functionality with UseProxyless for clearer intent
  • Adds dynamic configuration capability through function-based options
  • Updates dialer logic to respect both system capabilities and user preferences

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
option.go Adds WithUseProxyless option to configure proxyless dialing behavior
flashlight.go Integrates useProxyless field and passes it to client initialization
dialer/parallel_dialer.go Updates dialing logic to check both system support and user preference
dialer/dialer.go Replaces ProxyAll with UseProxyless in options struct
client/client.go Adds useProxyless field and integrates it into dialer initialization

Comment on lines +26 to 27
if !common.SupportsProxyless() || !d.opts.UseProxyless() {
// If the proxyless transport is not supported, we fall back to the default dialer.
Copy link
Preview

Copilot AI Jul 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition logic appears incorrect. The function should fall back to default dialer when proxyless is NOT supported OR when proxyless is NOT enabled. However, the function name UseProxyless suggests it returns true when proxyless should be used, making the negation !d.opts.UseProxyless() potentially confusing. Consider verifying the intended behavior: should this be !d.opts.UseProxyless() (fall back when proxyless is disabled) or d.opts.UseProxyless() == false?

Suggested change
if !common.SupportsProxyless() || !d.opts.UseProxyless() {
// If the proxyless transport is not supported, we fall back to the default dialer.
if !common.SupportsProxyless() || d.opts.UseProxyless() == false {
// If proxyless transport is not supported or not enabled, fall back to the default dialer.

Copilot uses AI. Check for mistakes.

socksWg sync.WaitGroup

DNSResolutionMapForDirectDialsEventual eventual.Value
useProxyless func() bool
Copy link
Preview

Copilot AI Jul 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The useProxyless field is added to the Client struct but there's no nil check before using it in initDialers. If useProxyless is nil when passed to UseProxyless, this could cause a panic when the function is called.

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant