cleanup: replace dial with newclient #8602

vinothkumarr227 · 2025-09-23T13:31:25Z

RELEASE NOTES: None

…s-tests-newclient

codecov · 2025-09-23T13:35:17Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.87%. Comparing base (7235bb7) to head (40a938e).
⚠️ Report is 35 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8602      +/-   ##
==========================================
+ Coverage   81.86%   81.87%   +0.01%     
==========================================
  Files         415      416       +1     
  Lines       40694    40789      +95     
==========================================
+ Hits        33316    33398      +82     
- Misses       5993     6022      +29     
+ Partials     1385     1369      -16

see 57 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

clientconn_parsed_target_test.go

arjan-bal · 2025-09-25T08:38:57Z

clientconn_parsed_target_test.go

-		"",
-		"unix://a/b/c",
-		"unix://authority",
-		"unix-abstract://authority/a/b/c",
-		"unix-abstract://authority",


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:

Call cc.Connect()

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.

If context times out before TF state, fail the test.

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

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.

Two options:

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

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.

I've raised a PR with the proposed change: #8643.

@vinothkumarr227 you can leave this test unchanged in this PR, I'll update the test at part of #8643.

After investigating, I confirmed that RPCs already fail with the correct error message. The connection is attempted when the stream creation function calls exitIdle, as seen here:

grpc-go/stream.go

Lines 180 to 186 in 2d92271

func newClientStream(ctx context.Context, desc *StreamDesc, cc *ClientConn, method string, opts ...CallOption) (_ ClientStream, err error) {

// Start tracking the RPC for idleness purposes. This is where a stream is

// created for both streaming and unary RPCs, and hence is a good place to

// track active RPC count.

if err := cc.idlenessMgr.OnCallBegin(); err != nil {

return nil, err

}

Therefore, no changes are needed to propagate the error. While cc.Connect itself cannot return an error due to its signature, any resolver build failure is correctly surfaced to the user during the next RPC.

This test can be changes as follows to create a new stream and validate the error.

cc, err := NewClient(target, WithTransportCredentials(insecure.NewCredentials())) if err != nil { t.Fatalf("NewClient(%q) failed: %v", target, err) } defer cc.Close() wantErrSubstr := "failed to exit idle mode" if _, err := cc.NewStream(ctx, &StreamDesc{}, "/my.service.v1.MyService/UnaryCall"); err == nil { t.Fatalf("NewStream() succeeded with target = %q, cc.parsedTarget = %+v, expected to fail", target, cc.parsedTarget) } else if !strings.Contains(err.Error(), wantErrSubstr) { t.Fatalf("NewStream() with target = %q returned unexpected error: got %v, want substring %q", target, err, wantErrSubstr) }

Thanks for the suggestion. I have modified the code changes accordingly

In the team meeting, we discussed this and decided that we should keep the origin test that uses Dial and add another test that uses NewClient. Can you please add the original test back, keeping the updated one also?

clientconn_parsed_target_test.go

clientconn_test.go

default_dial_option_server_option_test.go

resolver_test.go

eshitachandwani · 2025-10-02T09:09:05Z

clientconn_parsed_target_test.go

+			ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+			defer cancel()
+			cc, err := NewClient(target, WithTransportCredentials(insecure.NewCredentials()))
+			if err != nil {


We are changing the check condition here , is it intentional? If it is , the error message isn't correct. We are checking err!=nil which means NewClient failed and the error message says NewClient succeeded which is not right.

eshitachandwani · 2025-10-02T19:39:41Z

clientconn_parsed_target_test.go

 			}

-			cc, err := Dial(test.target, WithTransportCredentials(insecure.NewCredentials()), WithContextDialer(dialer))
+			cc, err := NewClient(test.target, WithTransportCredentials(insecure.NewCredentials()), withDefaultScheme(defScheme), WithContextDialer(dialer))


I don't think we should override the default scheme to passthrough with new client because we want to check the behavior of parsed strings with defualt NewClient function with custom dialer. And the behavior might be different , because dns resolver also does its own parsing , so the results might be different. Correct me if I have understood incorrectly. But I think we should have a separate test for NewClient's default parsed address behavior with custom dialer

Thanks for your feedback, I used withDefaultScheme only in the specific tests where it’s needed, as suggested by Arjan. For checking the default NewClient behavior with a custom dialer, I’m not overriding the scheme. If you’d like a separate test specifically for NewClient's default behavior, let me know and I can add it.

eshitachandwani · 2025-10-02T19:40:41Z

clientconn_test.go

 	if err != nil {
 		t.Fatalf("Dial failed. Err: %v", err)
 	}
+	client.Connect()


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

No need to change, I reverted it back.

arjan-bal · 2025-10-06T08:03:58Z

clientconn_parsed_target_test.go

-		"",
-		"unix://a/b/c",
-		"unix://authority",
-		"unix-abstract://authority/a/b/c",
-		"unix-abstract://authority",


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.

clientconn_parsed_target_test.go

resolver_test.go

arjan-bal

Mostly LGTM. Please address the pending comments and I'll add a second reviewer.

arjan-bal · 2025-10-16T04:31:18Z

default_dial_option_server_option_test.go

-		t.Fatalf("Dialing without a credential did not fail")
+	// Ensure the NewClient fails without credentials
+	if _, err := NewClient("fake"); err == nil {
+		t.Fatalf("grpc.NewClient without a credential did not fail")


nit: Please use NewClient instead of grpc.NewClient in the log, i.e. omit the package name. Change this throughout.

default_dial_option_server_option_test.go

arjan-bal · 2025-10-16T04:36:56Z

clientconn_parsed_target_test.go

-		"",
-		"unix://a/b/c",
-		"unix://authority",
-		"unix-abstract://authority/a/b/c",
-		"unix-abstract://authority",


In the team meeting, we discussed this and decided that we should keep the origin test that uses Dial and add another test that uses NewClient. Can you please add the original test back, keeping the updated one also?

arjan-bal · 2025-10-17T06:18:44Z

default_dial_option_server_option_test.go

 	noTSecStr := "no transport security set"
 	if _, err := NewClient("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)
+		t.Fatalf("NewClient received unexpected error: %q, want error containing %q", err, noTSecStr)


Please format errors with %v, %q should be used for strings.

arjan-bal · 2025-10-17T06:19:01Z

default_dial_option_server_option_test.go

 	noTSecStr := "no transport security set"
 	if _, err := NewClient("dns:///fake"); !strings.Contains(fmt.Sprint(err), noTSecStr) {
-		t.Fatalf("grpc.NewClient received unexpected error: %v, want error containing \"%v\"", err, noTSecStr)
+		t.Fatalf("NewClient received unexpected error: %q, want error containing %q", err, noTSecStr)


Please format errors with %v, %q should be used for strings.

vinothkumarr227 added 5 commits September 23, 2025 02:14

Updated NewClient instead of Dial

65f9bbe

Merge remote-tracking branch 'origin/master' into update-docs-example…

578671c

…s-tests-newclient

Fixed the test issues

c83de4b

small tweaks

714bd88

small tweaks

b060b0c

arjan-bal reviewed Sep 25, 2025

View reviewed changes

arjan-bal assigned vinothkumarr227 Sep 25, 2025

arjan-bal added the Type: Internal Cleanup Refactors, etc label Sep 25, 2025

arjan-bal added this to the 1.77 Release milestone Sep 25, 2025

vinothkumarr227 added 3 commits September 30, 2025 12:41

Fixed the review changes

98695b5

small tweaks

1a53f2b

small tweaks

889cf0c

eshitachandwani reviewed Oct 2, 2025

View reviewed changes

Fixed the review changes

66a1b3c

vinothkumarr227 removed their assignment Oct 6, 2025

arjan-bal mentioned this pull request Oct 6, 2025

Update docs and examples and tests to use NewClient instead of Dial #7049

Open

3 tasks

arjan-bal self-assigned this Oct 6, 2025

arjan-bal reviewed Oct 6, 2025

View reviewed changes

arjan-bal assigned dfawley and vinothkumarr227 and unassigned arjan-bal Oct 6, 2025

dfawley removed their assignment Oct 6, 2025

small tweaks

f1c77ee

easwars added the Status: Requires Reporter Clarification label Oct 7, 2025

vinothkumarr227 removed their assignment Oct 8, 2025

easwars assigned arjan-bal Oct 10, 2025

easwars removed the Status: Requires Reporter Clarification label Oct 10, 2025

arjan-bal mentioned this pull request Oct 13, 2025

grpc: Move channel to TRANSIENT_FAILURE on resolver creation failure #8643

Closed

arjan-bal removed their assignment Oct 13, 2025

arjan-bal assigned vinothkumarr227 Oct 13, 2025

small tweaks

115b4f7

vinothkumarr227 removed their assignment Oct 13, 2025

arjan-bal assigned vinothkumarr227 Oct 14, 2025

Fixed the test issues

f4de7e2

vinothkumarr227 removed their assignment Oct 14, 2025

arjan-bal self-assigned this Oct 16, 2025

arjan-bal reviewed Oct 16, 2025

View reviewed changes

arjan-bal approved these changes Oct 16, 2025

View reviewed changes

arjan-bal assigned vinothkumarr227 and unassigned arjan-bal Oct 16, 2025

Fixed the review changes

02196ff

vinothkumarr227 removed their assignment Oct 16, 2025

arjan-bal self-assigned this Oct 17, 2025

arjan-bal approved these changes Oct 17, 2025

View reviewed changes

arjan-bal assigned easwars and vinothkumarr227 and unassigned arjan-bal Oct 17, 2025

small tweaks

40a938e

vinothkumarr227 removed their assignment Oct 17, 2025

	func newClientStream(ctx context.Context, desc StreamDesc, cc ClientConn, method string, opts ...CallOption) (_ ClientStream, err error) {
	// Start tracking the RPC for idleness purposes. This is where a stream is
	// created for both streaming and unary RPCs, and hence is a good place to
	// track active RPC count.
	if err := cc.idlenessMgr.OnCallBegin(); err != nil {
	return nil, err
	}

cleanup: replace dial with newclient #8602

Are you sure you want to change the base?

cleanup: replace dial with newclient #8602

Conversation

vinothkumarr227 commented Sep 23, 2025

Uh oh!

codecov bot commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

arjan-bal 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!

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Sep 23, 2025 •

edited

Loading