Skip to content

cleanup: replace dial with newclient #8196

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.

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 11 commits into
base: master
Choose a base branch
from
4 changes: 2 additions & 2 deletions Documentation/encoding.md
Original file line number Diff line number Diff line change
@@ -57,7 +57,7 @@ As a reminder, all `CallOption`s may be converted into `DialOption`s that become
the default for all RPCs sent through a client using `grpc.WithDefaultCallOptions`:

```go
myclient := grpc.Dial(ctx, target, grpc.WithDefaultCallOptions(grpc.CallContentSubtype("mycodec")))
myclient := grpc.NewClient(target, grpc.WithDefaultCallOptions(grpc.CallContentSubtype("mycodec")))
```

When specified in either of these ways, messages will be encoded using this
@@ -132,7 +132,7 @@ As a reminder, all `CallOption`s may be converted into `DialOption`s that become
the default for all RPCs sent through a client using `grpc.WithDefaultCallOptions`:

```go
myclient := grpc.Dial(ctx, target, grpc.WithDefaultCallOptions(grpc.UseCompressor("gzip")))
myclient := grpc.NewClient(target, grpc.WithDefaultCallOptions(grpc.UseCompressor("gzip")))
```

When specified in either of these ways, messages will be compressed using this
6 changes: 3 additions & 3 deletions Documentation/grpc-auth-support.md
Original file line number Diff line number Diff line change
@@ -5,7 +5,7 @@ As outlined in the [gRPC authentication guide](https://grpc.io/docs/guides/auth.
# Enabling TLS on a gRPC client

```Go
conn, err := grpc.Dial(serverAddr, grpc.WithTransportCredentials(credentials.NewClientTLSFromCert(nil, "")))
conn, err := grpc.NewClient(serverAddr, grpc.WithTransportCredentials(credentials.NewClientTLSFromCert(nil, "")))
```

# Enabling TLS on a gRPC server
@@ -63,7 +63,7 @@ to prevent any insecure transmission of tokens.
## Google Compute Engine (GCE)

```Go
conn, err := grpc.Dial(serverAddr, grpc.WithTransportCredentials(credentials.NewClientTLSFromCert(nil, "")), grpc.WithPerRPCCredentials(oauth.NewComputeEngine()))
conn, err := grpc.NewClient(serverAddr, grpc.WithTransportCredentials(credentials.NewClientTLSFromCert(nil, "")), grpc.WithPerRPCCredentials(oauth.NewComputeEngine()))
```

## JWT
@@ -73,6 +73,6 @@ jwtCreds, err := oauth.NewServiceAccountFromFile(*serviceAccountKeyFile, *oauthS
if err != nil {
log.Fatalf("Failed to create JWT credentials: %v", err)
}
conn, err := grpc.Dial(serverAddr, grpc.WithTransportCredentials(credentials.NewClientTLSFromCert(nil, "")), grpc.WithPerRPCCredentials(jwtCreds))
conn, err := grpc.NewClient(serverAddr, grpc.WithTransportCredentials(credentials.NewClientTLSFromCert(nil, "")), grpc.WithPerRPCCredentials(jwtCreds))
```

5 changes: 2 additions & 3 deletions balancer/endpointsharding/endpointsharding_test.go
Original file line number Diff line number Diff line change
@@ -23,7 +23,6 @@ import (
"encoding/json"
"errors"
"fmt"
"log"
"strings"
"testing"
"time"
@@ -166,7 +165,7 @@ func (s) TestEndpointShardingBasic(t *testing.T) {
}
cc, err := grpc.NewClient(mr.Scheme()+":///", dOpts...)
if err != nil {
log.Fatalf("Failed to create new client: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please only change log to t, the error message is fine.

t.Fatalf("Failed to create new client: %v", err)
}
defer cc.Close()
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
@@ -246,7 +245,7 @@ func (s) TestEndpointShardingReconnectDisabled(t *testing.T) {

cc, err := grpc.NewClient(mr.Scheme()+":///", grpc.WithResolvers(mr), grpc.WithTransportCredentials(insecure.NewCredentials()))
if err != nil {
log.Fatalf("Failed to create new client: %v", err)
t.Fatalf("Failed to create new client: %v", err)
}
defer cc.Close()
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
2 changes: 1 addition & 1 deletion credentials/insecure/insecure.go
Original file line number Diff line number Diff line change
@@ -30,7 +30,7 @@ import (
// NewCredentials returns a credentials which disables transport security.
//
// Note that using this credentials with per-RPC credentials which require
// transport security is incompatible and will cause grpc.Dial() to fail.
// transport security is incompatible and will cause grpc.NewClient() to fail.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you verify that grpc.NewClient fails? Or does the failure only surface when the channel exits idle state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

grpc.NewClient() itself doesn't immediately fail when using insecure credentials with per-RPC credentials that require transport security.
The failure actually happens later — when the channel attempts to connect when exiting idle or making an RPC.

func NewCredentials() credentials.TransportCredentials {
return insecureTC{}
}
2 changes: 1 addition & 1 deletion dialoptions.go
Original file line number Diff line number Diff line change
@@ -360,7 +360,7 @@ func WithReturnConnectionError() DialOption {
//
// Note that using this DialOption with per-RPC credentials (through
// WithCredentialsBundle or WithPerRPCCredentials) which require transport
// security is incompatible and will cause grpc.Dial() to fail.
// security is incompatible and will cause grpc.NewClient() to fail.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you verify that grpc.NewClient fails? Or does the failure only surface when the channel exits idle state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rpc.NewClient() itself doesn't immediately fail when using insecure credentials with per-RPC credentials that require transport security.
The failure actually happens later — when the channel attempts to connect when exiting idle or making an RPC.

//
// Deprecated: use WithTransportCredentials and insecure.NewCredentials()
// instead. Will be supported throughout 1.x.
3 changes: 1 addition & 2 deletions internal/stats/metrics_recorder_list_test.go
Original file line number Diff line number Diff line change
@@ -23,7 +23,6 @@ package stats_test
import (
"context"
"fmt"
"log"
"strings"
"testing"
"time"
@@ -154,7 +153,7 @@ func (s) TestMetricsRecorderList(t *testing.T) {

cc, err := grpc.NewClient(mr.Scheme()+":///", grpc.WithResolvers(mr), grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithStatsHandler(mr2))
if err != nil {
log.Fatalf("Failed to dial: %v", err)
t.Fatalf("grpc.NewClient() failed: %v", err)
}
defer cc.Close()

10 changes: 7 additions & 3 deletions stats/stats_test.go
Original file line number Diff line number Diff line change
@@ -30,6 +30,7 @@ import (

"github.com/google/go-cmp/cmp"
"google.golang.org/grpc"
"google.golang.org/grpc/connectivity"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/internal"
"google.golang.org/grpc/internal/grpctest"
@@ -274,7 +275,6 @@ func (te *test) clientConn() *grpc.ClientConn {
}
opts := []grpc.DialOption{
grpc.WithTransportCredentials(insecure.NewCredentials()),
grpc.WithBlock(),
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are removing WithBlock, you need to ensure the clientconn is ready before the function returns it. I ssupect this is the reason for the race conditions that you're seeing. Please use cc.Connect and testutils.AwaitState here to restore the behviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out — you're absolutely right. Removing WithBlock without ensuring the connection is ready was likely the cause of the race conditions. I've now updated the code to call cc.Connect() and use testutils.AwaitState to properly wait for Ready before returning. Apologies for overlooking this earlier!

grpc.WithUserAgent("test/0.0.1"),
}
if te.compress == "gzip" {
@@ -288,10 +288,14 @@ func (te *test) clientConn() *grpc.ClientConn {
}

var err error
te.cc, err = grpc.Dial(te.srvAddr, opts...)
te.cc, err = grpc.NewClient(te.srvAddr, opts...)
if err != nil {
te.t.Fatalf("Dial(%q) = %v", te.srvAddr, err)
te.t.Fatalf("grpc.NewClient() failed(%q) = %v", te.srvAddr, err)
}
te.cc.Connect()
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()
testutils.AwaitState(ctx, te.t, te.cc, connectivity.Ready)
return te.cc
}

6 changes: 3 additions & 3 deletions test/channelz_test.go
Original file line number Diff line number Diff line change
@@ -304,11 +304,11 @@ func (s) TestCZTopChannelRegistrationAndDeletion(t *testing.T) {
}
}

func (s) TestCZTopChannelRegistrationAndDeletionWhenDialFail(t *testing.T) {
// Make dial fails (due to no transport security specified)
func (s) TestCZTopChannelRegistrationAndDeletionWhenNewClientFail(t *testing.T) {
// Make newclient fails (due to no transport security specified)
_, err := grpc.NewClient("fake.addr")
if err == nil {
t.Fatal("expecting dial to fail")
t.Fatal("expecting newclient to fail")
}
if tcs, end := channelz.GetTopChannels(0, 0); tcs != nil || !end {
t.Fatalf("GetTopChannels(0, 0) = %v, %v, want <nil>, true", tcs, end)
5 changes: 3 additions & 2 deletions test/end2end_test.go
Original file line number Diff line number Diff line change
@@ -853,10 +853,11 @@ func (te *test) clientConn(opts ...grpc.DialOption) *grpc.ClientConn {
var scheme string
opts, scheme = te.configDial(opts...)
var err error
te.cc, err = grpc.Dial(scheme+te.srvAddr, opts...)
te.cc, err = grpc.NewClient(scheme+te.srvAddr, opts...)
if err != nil {
te.t.Fatalf("Dial(%q) = %v", scheme+te.srvAddr, err)
te.t.Fatalf("grpc.NewClient() failed(%q) = %v", scheme+te.srvAddr, err)
}
te.cc.Connect()
return te.cc
}