Dial using proxyless in parallel with proxies #1492

myleshorton · 2025-04-14T18:51:16Z

This first always tries to dial directly using proxyless if we can.

Copilot

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

go.mod: Language not supported

dialer/proxyless.go

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

myleshorton · 2025-04-14T19:10:17Z

Gotta fix one quirk with the outline stuff.

WendelHime · 2025-04-14T19:20:47Z

dialer/proxyless.go

+	configBytes, err := embedFS.ReadFile("smart_dialer_config.yml")
+	if err != nil {
+		log.Errorf("Failed to read smart dialer config: %v", err)
+		return nil, err
+	}


We probably want to load this file once instead of reading every dial, so we could move this to newProxylessDialer and add a attribute for config on proxylessDialer

I guess it would be interesting to have a map of proxyless techniques per country and maybe provide them at the global config, but for now this is good

@myleshorton what do you think about adding the smart dialer config as part of the global config? We could add different techniques later and it wouldn't be hard-coded

I'm afraid that users in the future might stop updating on this version (by any random reason which happens) and the techniques implemented here would be frozen

garmr-ulfr · 2025-04-14T19:50:41Z

dialer/proxyless.go

+	var addr string
+	var host string
+	var port string
+	var err error
+	if host, port, err = net.SplitHostPort(address); err != nil {
+		addr = fmt.Sprintf("%s:%s", host, "443")
+	} else if port == "" || port == "443" {
+		addr = fmt.Sprintf("%s:%s", host, "443")
+	} else {
+		return nil, fmt.Errorf("proxyless can only dial 443: %s", addr)
+	}


It's not really important but, this can all be simplified to:

host, port, _ := net.SplitHostPort(address) if port != "" && port != "443" { return nil, fmt.Errorf("proxyless can only dial 443: %s", addr) } addr := host + ":443"

garmr-ulfr

Just a couple of comments, but otherwise LGTM!

garmr-ulfr · 2025-04-14T20:10:45Z

dialer/proxyless.go

+	}
+
+	domains := []string{host}
+	dialer, err := d.strategyFinder.NewDialer(context.Background(), domains, d.configBytes)


Shouldn't we also use the provided ctx here?

myleshorton · 2025-04-15T00:40:52Z

I'm taking another pass at this to parallelize it...will have it in tomorrow!

Copilot

Pull Request Overview

This PR refactors the dialing logic to prioritize direct dialing via proxyless connections while maintaining fallbacks using a parallel dialer approach. Key changes include removing the two-phase dialer implementation, introducing a new smart dialer configuration file, and updating various dialer and client components to integrate the new approach.

Reviewed Changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
dialer/two_phase_dialer.go	Removed obsolete two-phase dialer implementation
dialer/smart_dialer_config.yml	Added new configuration for smart/proxyless dialing
dialer/proxyless.go	Introduced proxylessDialer with enhanced error handling and reset
dialer/parallel_dialer.go	Added a parallel dialer that chooses between proxyless and fallback
dialer/fastconnect.go	Updated cancellation logging in parallelDial routine
dialer/dialer.go	Refactored dialer creation to prioritize direct dialing
dialer/bandit.go	Minor update to error messaging in bandit dialer
client/client_test.go	Adjusted error message expectations in tests
client/client.go	Improved cancellation logging for dial contexts
.github/workflows/go.yml	Updated test workflow to remove deprecated reporter usage

Files not reviewed (1)

go.mod: Language not supported

Comments suppressed due to low confidence (1)

dialer/dialer.go:49

[nitpick] The variable name 'pd' is ambiguous; consider renaming it to 'parallelDialer' to improve clarity.

pd := newParallelPreferProxyless(pdialer, fcd)

myleshorton · 2025-04-16T18:45:54Z

.github/workflows/go.yml

        with:
          go-version-file: "go.mod"
-      - name: Install go go-ctrf-json-reporter
-        run: go install github.com/ctrf-io/go-ctrf-json-reporter/cmd/go-ctrf-json-reporter@latest


They updated this repo/GH action, so this started failing.

myleshorton · 2025-04-16T20:26:51Z

Hey @WendelHime and @garmr-ulfr I ended up making a bunch of changes here to try proxyless in parallel to make sure this only improves upon the current situation for users. I ended up giving the proxyless dialers their own context because otherwise the context from the proxied dialer would cancel the proxyless whenever it succeeded first.

garmr-ulfr · 2025-04-16T20:56:18Z

Reviewing now

myleshorton · 2025-04-16T21:30:14Z

I just realized I think I lost track of the state change in there -- i.e. FastConnect has the logic to switch to the Bandit, but Bandit doesn't actually get used I don't think! Will fix!

garmr-ulfr · 2025-04-16T20:55:21Z

client/client_test.go

 	require.NoError(t, err)
 	resp, err := roundTrip(client, req)
-	require.Contains(t, err.Error(), "deadline")
+	require.Contains(t, err.Error(), "any dialers")


Is error non-nil on success?

garmr-ulfr · 2025-04-16T22:59:53Z

dialer/parallel_dialer.go

+			if err != nil {
+				errCh <- err
+			}
+			connCh <- conn


This should be wrapped in an else. Currently, it still sends a value (nil) on error and so the following select statement will sometimes read from connCh, instead of errCh, returning nil,nil.

oooh good catch!!

Copilot

Pull Request Overview

This PR refactors the dialing logic to always attempt a direct proxyless connection first before falling back to traditional proxied dialing. Key changes include:

Removal of the two-phase dialer implementation.
Introduction of a dedicated proxyless dialer and corresponding configuration.
Updates to fast connect, parallel dialer, and client initialization to support the new proxyless-first approach.

Reviewed Changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
dialer/two_phase_dialer.go	Removed obsolete two-phase dialing logic.
dialer/smart_dialer_config.yml	Added configuration for smart/proxyless dialing.
dialer/proxyless_test.go	Added tests for proxyless dialer behavior.
dialer/proxyless.go	Introduced proxyless dialer implementation with error handling.
dialer/parallel_dialer.go	Added parallel dialing combining proxyless and default dialers.
dialer/fastconnect_test.go	Updated tests to match refactored fast connect dialer signature.
dialer/fastconnect.go	Modified fast connect dialer initialization and dialer switch logic.
dialer/dialer.go	Updated dialer initialization to use the new proxyless dialer.
dialer/bandit.go	Adjusted bandit dialer options to align with the new proxyless strategy.
client/client_test.go	Updated expectations to reflect the revised error message wording.
client/client.go	Modified client dialer integration and improved context cancellation.
.github/workflows/go.yml	Simplified the Go workflow by removing extra test report steps.

Files not reviewed (1)

go.mod: Language not supported

dialer/parallel_dialer.go

Copilot

Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

go.mod: Language not supported

dialer/parallel_dialer.go

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

myleshorton added 2 commits April 14, 2025 12:44

Try proxyless first

e7b535c

Always try proxyless first

938946d

myleshorton requested a review from Copilot April 14, 2025 18:51

Copilot AI reviewed Apr 14, 2025

View reviewed changes

dialer/proxyless.go Outdated Show resolved Hide resolved

myleshorton and others added 2 commits April 14, 2025 12:53

Update dialer/proxyless.go

4d7fed9

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Fix tests

42a6670

myleshorton requested review from WendelHime and garmr-ulfr April 14, 2025 19:05

myleshorton marked this pull request as draft April 14, 2025 19:10

Fixed port handling

546f97e

WendelHime reviewed Apr 14, 2025

View reviewed changes

myleshorton marked this pull request as ready for review April 14, 2025 19:49

garmr-ulfr reviewed Apr 14, 2025

View reviewed changes

garmr-ulfr approved these changes Apr 14, 2025

View reviewed changes

myleshorton marked this pull request as draft April 15, 2025 01:01

myleshorton added 5 commits April 16, 2025 11:56

Dial proxyless in parallel and then cache success and failure for re-use

e2cc803

Fixed test

10e210f

simplify test

a0352b7

no json in test output

8ebb1e1

fixed test

ee40ae6

myleshorton requested a review from Copilot April 16, 2025 18:35

Copilot AI reviewed Apr 16, 2025

View reviewed changes

myleshorton marked this pull request as ready for review April 16, 2025 18:37

myleshorton added 3 commits April 16, 2025 12:39

cleanup

05654d2

Fixed comment

73d97cb

call close on inner dialer

860ea33

myleshorton commented Apr 16, 2025

View reviewed changes

myleshorton changed the title ~~Always try to dial directly first~~ Dial using proxyless in parallel with proxies Apr 16, 2025

use latest kindling and fronted

f94c321

myleshorton requested review from garmr-ulfr and WendelHime April 16, 2025 20:25

garmr-ulfr reviewed Apr 17, 2025

View reviewed changes

myleshorton added 4 commits April 17, 2025 11:16

Cleaner state machine for dialers

15afae6

Code review fix

a7c7d2d

check for nil callback func

6b81b12

More dialer simplifications

3675b27

myleshorton requested a review from Copilot April 17, 2025 17:43

Copilot AI reviewed Apr 17, 2025

View reviewed changes

dialer/parallel_dialer.go Outdated Show resolved Hide resolved

myleshorton added 5 commits April 17, 2025 11:49

cleanup parallel select

d305227

revert to proxyless state on options

5b44cb1

do not print config every time

681073b

Fixed test

d1ca81c

more cleanups

a9f1c21

myleshorton requested a review from Copilot April 17, 2025 19:27

Copilot AI reviewed Apr 17, 2025

View reviewed changes

dialer/parallel_dialer.go Outdated Show resolved Hide resolved

Update dialer/parallel_dialer.go

2b73849

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

myleshorton merged commit 99d66de into main Apr 17, 2025
3 checks passed

myleshorton deleted the myles/proxyless branch April 17, 2025 19:42

Dial using proxyless in parallel with proxies #1492

Dial using proxyless in parallel with proxies #1492

Uh oh!

Conversation

myleshorton commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

myleshorton commented Apr 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garmr-ulfr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myleshorton commented Apr 15, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myleshorton commented Apr 16, 2025

Uh oh!

garmr-ulfr commented Apr 16, 2025

Uh oh!

myleshorton commented Apr 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

myleshorton commented Apr 14, 2025 •

edited

Loading