Skip to content

cleanup: replace dial with newclient #8602

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

cleanup: replace dial with newclient #8602

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
65f9bbe
Updated NewClient instead of Dial
vinothkumarr227 Sep 23, 2025
578671c
Merge remote-tracking branch 'origin/master' into update-docs-example…
vinothkumarr227 Sep 23, 2025
c83de4b
Fixed the test issues
vinothkumarr227 Sep 23, 2025
714bd88
small tweaks
vinothkumarr227 Sep 23, 2025
b060b0c
small tweaks
vinothkumarr227 Sep 23, 2025
98695b5
Fixed the review changes
vinothkumarr227 Sep 30, 2025
1a53f2b
small tweaks
vinothkumarr227 Sep 30, 2025
889cf0c
small tweaks
vinothkumarr227 Sep 30, 2025
66a1b3c
Fixed the review changes
vinothkumarr227 Oct 3, 2025
f1c77ee
small tweaks
vinothkumarr227 Oct 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions clientconn_authority_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,9 @@ func (s) TestClientConnAuthority(t *testing.T) {

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
cc, err := Dial(test.target, test.opts...)
cc, err := NewClient(test.target, test.opts...)
if err != nil {
t.Fatalf("Dial(%q) failed: %v", test.target, err)
t.Fatalf("grpc.NewClient(%q) failed: %v", test.target, err)
}
defer cc.Close()
if cc.authority != test.wantAuthority {
Expand Down
60 changes: 13 additions & 47 deletions clientconn_parsed_target_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,74 +62,63 @@ func init() { resolver.Register(testResolverForParser{}) }
func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) {
tests := []struct {
target string
wantDialParse resolver.Target
wantNewClientParse resolver.Target
wantCustomParse resolver.Target
}{
// No scheme is specified.
{
target: "://a/b",
wantDialParse: generateTarget("passthrough:///://a/b"),
wantNewClientParse: generateTarget("dns:///://a/b"),
wantCustomParse: generateTarget("testresolverforparser:///://a/b"),
},
{
target: "a//b",
wantDialParse: generateTarget("passthrough:///a//b"),
wantNewClientParse: generateTarget("dns:///a//b"),
wantCustomParse: generateTarget("testresolverforparser:///a//b"),
},

// An unregistered scheme is specified.
{
target: "a:///",
wantDialParse: generateTarget("passthrough:///a:///"),
wantNewClientParse: generateTarget("dns:///a:///"),
wantCustomParse: generateTarget("testresolverforparser:///a:///"),
},
{
target: "a:b",
wantDialParse: generateTarget("passthrough:///a:b"),
wantNewClientParse: generateTarget("dns:///a:b"),
wantCustomParse: generateTarget("testresolverforparser:///a:b"),
},

// A registered scheme is specified.
{
target: "dns://a.server.com/google.com",
wantDialParse: generateTarget("dns://a.server.com/google.com"),
wantNewClientParse: generateTarget("dns://a.server.com/google.com"),
wantCustomParse: generateTarget("dns://a.server.com/google.com"),
},
{
target: "unix-abstract:/ a///://::!@#$%25^&*()b",
wantDialParse: generateTarget("unix-abstract:/ a///://::!@#$%25^&*()b"),
wantNewClientParse: generateTarget("unix-abstract:/ a///://::!@#$%25^&*()b"),
wantCustomParse: generateTarget("unix-abstract:/ a///://::!@#$%25^&*()b"),
},
{
target: "unix-abstract:passthrough:abc",
wantDialParse: generateTarget("unix-abstract:passthrough:abc"),
wantNewClientParse: generateTarget("unix-abstract:passthrough:abc"),
wantCustomParse: generateTarget("unix-abstract:passthrough:abc"),
},
{
target: "passthrough:///unix:///a/b/c",
wantDialParse: generateTarget("passthrough:///unix:///a/b/c"),
wantNewClientParse: generateTarget("passthrough:///unix:///a/b/c"),
wantCustomParse: generateTarget("passthrough:///unix:///a/b/c"),
},

// Cases for `scheme:absolute-path`.
{
target: "dns:/a/b/c",
wantDialParse: generateTarget("dns:/a/b/c"),
wantNewClientParse: generateTarget("dns:/a/b/c"),
wantCustomParse: generateTarget("dns:/a/b/c"),
},
{
target: "unregistered:/a/b/c",
wantDialParse: generateTarget("passthrough:///unregistered:/a/b/c"),
wantNewClientParse: generateTarget("dns:///unregistered:/a/b/c"),
wantCustomParse: generateTarget("testresolverforparser:///unregistered:/a/b/c"),
},
Expand All @@ -138,19 +127,9 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) {
for _, test := range tests {
t.Run(test.target, func(t *testing.T) {
resetInitialResolverState()
cc, err := Dial(test.target, WithTransportCredentials(insecure.NewCredentials()))
cc, err := NewClient(test.target, WithTransportCredentials(insecure.NewCredentials()))
if err != nil {
t.Fatalf("Dial(%q) failed: %v", test.target, err)
}
cc.Close()

if !cmp.Equal(cc.parsedTarget, test.wantDialParse) {
t.Errorf("cc.parsedTarget for dial target %q = %+v, want %+v", test.target, cc.parsedTarget, test.wantDialParse)
}

cc, err = NewClient(test.target, WithTransportCredentials(insecure.NewCredentials()))
if err != nil {
t.Fatalf("NewClient(%q) failed: %v", test.target, err)
t.Fatalf("grpc.NewClient(%q) failed: %v", test.target, err)
}
cc.Close()

Expand All @@ -159,19 +138,9 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) {
}

resolver.SetDefaultScheme("testresolverforparser")
cc, err = Dial(test.target, WithTransportCredentials(insecure.NewCredentials()))
if err != nil {
t.Fatalf("Dial(%q) failed: %v", test.target, err)
}
cc.Close()

if !cmp.Equal(cc.parsedTarget, test.wantCustomParse) {
t.Errorf("cc.parsedTarget for dial target %q = %+v, want %+v", test.target, cc.parsedTarget, test.wantDialParse)
}

cc, err = NewClient(test.target, WithTransportCredentials(insecure.NewCredentials()))
if err != nil {
t.Fatalf("NewClient(%q) failed: %v", test.target, err)
t.Fatalf("grpc.NewClient(%q) failed: %v", test.target, err)
}
cc.Close()

Expand All @@ -186,18 +155,14 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) {

func (s) TestParsedTarget_Failure_WithoutCustomDialer(t *testing.T) {
targets := []string{
"",
"unix://a/b/c",
"unix://authority",
"unix-abstract://authority/a/b/c",
"unix-abstract://authority",
Comment on lines 189 to 193
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not remove these test cases. Since NewClient doesn't connect like Dial, we need to do the following to get an error instead:

  1. Call cc.Connect()
  2. Wait for TRANSIENT_FAILURE state using AwaitState. If using this function causes a circular dependency, copy it over to this file, adding a comment that mentioning the reason for the duplication.
  3. If context times out before TF state, fail the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using Idle because these malformed targets result in zero resolved addresses. The client never attempts a connection, so it stays IDLE and cannot reach TRANSIENT_FAILURE

Copy link
Contributor

Choose a reason for hiding this comment

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

There's an INFO log similar to the following from the unix resolver:

INFO clientconn.go:333 [core] [Channel #5] Channel failed to exit idle mode: invalid (non-empty) authority: authority (t=+1.01919ms)

Since this is the only indication that the connection attempt failed (no errors are returned), I think this should be logged as an ERROR. Presently, the method responsible for channelz trace event logging is logging everything at as an INFO log. We could have at accept the severity as an additional param to achieve this. Once this is an error log, we can assert that the error is logged similar to the following.

grpc-go/internal/transport/keepalive_test.go

Line 401 in 8389ddb

grpctest.ExpectError("Client received GoAway with error code ENHANCE_YOUR_CALM and debug data equal to ASCII \"too_many_pings\"")

@dfawley do you think we should log the failure to exit idle mode in cc.Connect() as an ERROR instead of INFO.

Copy link
Member

Choose a reason for hiding this comment

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

Two options:

  1. Change the unix name resolver not to error from Build when this happens. Instead, have it produce a ResolverError and otherwise do nothing.

  2. Handle resolver.Build errors so that all name resolvers that error during Build will instead effectively produce a ResolverError. This fixes that class of problem in the same way for all name resolvers.

Either way, RPCs will fail with that error message text, and we shouldn't need anything besides INFO (or even V2?) logs.

"unix:/%a/b/c",
}

for _, target := range targets {
t.Run(target, func(t *testing.T) {
if cc, err := Dial(target, WithTransportCredentials(insecure.NewCredentials())); err == nil {
if cc, err := NewClient(target, WithTransportCredentials(insecure.NewCredentials())); err == nil {
defer cc.Close()
t.Fatalf("Dial(%q) succeeded cc.parsedTarget = %+v, expected to fail", target, cc.parsedTarget)
t.Fatalf("grpc.NewClient(%q) succeeded cc.parsedTarget = %+v, expected to fail", target, cc.parsedTarget)
}
})
}
Expand Down Expand Up @@ -234,7 +199,7 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) {
wantDialerAddress: "127.0.0.1:50051",
},
{
target: ":///127.0.0.1:50051",
target: "passthrough:///:///127.0.0.1:50051",
wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, ":///127.0.0.1:50051"))},
wantDialerAddress: ":///127.0.0.1:50051",
},
Expand All @@ -244,17 +209,17 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) {
wantDialerAddress: "127.0.0.1:50051",
},
{
target: "://authority/127.0.0.1:50051",
target: "passthrough:///://authority/127.0.0.1:50051",
wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "://authority/127.0.0.1:50051"))},
wantDialerAddress: "://authority/127.0.0.1:50051",
},
{
target: "/unix/socket/address",
target: "passthrough:////unix/socket/address",
wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, "/unix/socket/address"))},
wantDialerAddress: "/unix/socket/address",
},
{
target: "",
target: "passthrough:///",
wantParsed: resolver.Target{URL: *testutils.MustParseURL(fmt.Sprintf("%s:///%s", defScheme, ""))},
wantDialerAddress: "",
},
Expand All @@ -273,11 +238,12 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) {
return nil, errors.New("dialer error")
}

cc, err := Dial(test.target, WithTransportCredentials(insecure.NewCredentials()), WithContextDialer(dialer))
cc, err := NewClient(test.target, WithTransportCredentials(insecure.NewCredentials()), WithContextDialer(dialer))
if err != nil {
t.Fatalf("Dial(%q) failed: %v", test.target, err)
t.Fatalf("grpc.NewClient(%q) failed: %v", test.target, err)
}
defer cc.Close()
cc.Connect()

select {
case addr := <-addrCh:
Expand Down
5 changes: 3 additions & 2 deletions clientconn_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,12 @@ func (s) TestDialWithTimeout(t *testing.T) {

r := manual.NewBuilderWithScheme("whatever")
r.InitialState(resolver.State{Addresses: []resolver.Address{lisAddr}})
client, err := Dial(r.Scheme()+":///test.server", WithTransportCredentials(insecure.NewCredentials()), WithResolvers(r), WithTimeout(5*time.Second))
client, err := NewClient(r.Scheme()+":///test.server", WithTransportCredentials(insecure.NewCredentials()), WithResolvers(r), WithTimeout(5*time.Second))
close(dialDone)
if err != nil {
t.Fatalf("Dial failed. Err: %v", err)
t.Fatalf("grpc.NewClient failed. Err: %v", err)
}
client.Connect()
Copy link
Member

Choose a reason for hiding this comment

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

Why did we add this here? We did not change from Dial to NewClient in the test and Dial will connect automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to change, I reverted it back.

defer client.Close()
timeout := time.After(1 * time.Second)
select {
Expand Down
26 changes: 13 additions & 13 deletions default_dial_option_server_option_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@ import (
)

func (s) TestAddGlobalDialOptions(t *testing.T) {
// Ensure the Dial fails without credentials
if _, err := Dial("fake"); err == nil {
t.Fatalf("Dialing without a credential did not fail")
// Ensure the NewClient fails without credentials
if _, err := NewClient("dns:///fake"); err == nil {
t.Fatalf("grpc.NewClient without a credential did not fail")
} else {
if !strings.Contains(err.Error(), "no transport security set") {
t.Fatalf("Dialing failed with unexpected error: %v", err)
t.Fatalf("grpc.NewClient failed with unexpected error: %v", err)
}
}

Expand All @@ -48,9 +48,9 @@ func (s) TestAddGlobalDialOptions(t *testing.T) {
}
}

// Ensure the Dial passes with the extra dial options
if cc, err := Dial("fake"); err != nil {
t.Fatalf("Dialing with insecure credential failed: %v", err)
// Ensure the NewClient passes with the extra dial options
if cc, err := NewClient("dns:///fake"); err != nil {
t.Fatalf("grpc.NewClient with insecure credential failed: %v", err)
} else {
cc.Close()
}
Expand All @@ -71,8 +71,8 @@ func (s) TestDisableGlobalOptions(t *testing.T) {
// due to the global dial options with credentials not being picked up due
// to global options being disabled.
noTSecStr := "no transport security set"
if _, err := Dial("fake", internal.DisableGlobalDialOptions.(func() DialOption)()); !strings.Contains(fmt.Sprint(err), noTSecStr) {
t.Fatalf("Dialing received unexpected error: %v, want error containing \"%v\"", err, noTSecStr)
if _, err := NewClient("dns:///fake", internal.DisableGlobalDialOptions.(func() DialOption)()); !strings.Contains(fmt.Sprint(err), noTSecStr) {
t.Fatalf("grpc.NewClient received unexpected error: %v, want error containing \"%v\"", err, noTSecStr)
}
}

Expand All @@ -95,11 +95,11 @@ func (s) TestGlobalPerTargetDialOption(t *testing.T) {
defer internal.ClearGlobalPerTargetDialOptions()
noTSecStr := "no transport security set"
if _, err := NewClient("dns:///fake"); !strings.Contains(fmt.Sprint(err), noTSecStr) {
t.Fatalf("Dialing received unexpected error: %v, want error containing \"%v\"", err, noTSecStr)
t.Fatalf("grpc.NewClient received unexpected error: %v, want error containing \"%v\"", err, noTSecStr)
}
cc, err := NewClient("passthrough:///nice")
if err != nil {
t.Fatalf("Dialing with insecure credentials failed: %v", err)
t.Fatalf("grpc.NewClient with insecure credentials failed: %v", err)
}
cc.Close()
}
Expand Down Expand Up @@ -135,9 +135,9 @@ func (s) TestJoinDialOption(t *testing.T) {
const maxRecvSize = 998765
const initialWindowSize = 100
jdo := newJoinDialOption(WithTransportCredentials(insecure.NewCredentials()), WithReadBufferSize(maxRecvSize), WithInitialWindowSize(initialWindowSize))
cc, err := Dial("fake", jdo)
cc, err := NewClient("dns:///fake", jdo)
if err != nil {
t.Fatalf("Dialing with insecure credentials failed: %v", err)
t.Fatalf("grpc.NewClient with insecure credentials failed: %v", err)
}
defer cc.Close()
if cc.dopts.copts.ReadBufferSize != maxRecvSize {
Expand Down
12 changes: 6 additions & 6 deletions resolver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ func (s) TestResolverCaseSensitivity(t *testing.T) {
return nil, fmt.Errorf("not dialing with custom dialer")
}

cc, err := Dial(target, WithContextDialer(customDialer), WithTransportCredentials(insecure.NewCredentials()))
cc, err := NewClient(target, WithContextDialer(customDialer), WithTransportCredentials(insecure.NewCredentials()))
if err != nil {
t.Fatalf("Unexpected Dial(%q) error: %v", target, err)
t.Fatalf("Unexpected grpc.NewClient(%q) error: %v", target, err)
}
cc.Connect()
if got, want := <-addrCh, "localhost:1234"; got != want {
Expand All @@ -84,13 +84,13 @@ func (s) TestResolverCaseSensitivity(t *testing.T) {
// This should not find the injected resolver due to the case not matching.
// This results in "passthrough" being used with the address as the whole
// target.
target = "caseTest2:///localhost:1234"
cc, err = Dial(target, WithContextDialer(customDialer), WithResolvers(res), WithTransportCredentials(insecure.NewCredentials()))
target = "passthrough:///caseTest2:///localhost:1234"
cc, err = NewClient(target, WithContextDialer(customDialer), WithResolvers(res), WithTransportCredentials(insecure.NewCredentials()))
if err != nil {
t.Fatalf("Unexpected Dial(%q) error: %v", target, err)
t.Fatalf("Unexpected grpc.NewClient(%q) error: %v", target, err)
}
cc.Connect()
if got, want := <-addrCh, target; got != want {
if got, want := <-addrCh, "caseTest2:///localhost:1234"; got != want {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we revert this change? It seems to be a no-op since the target variable has the same value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

cc.Close()
t.Fatalf("Dialer got address %q; wanted %q", got, want)
}
Expand Down
Loading