Skip to content
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

Deprecation of DialContext and Dial is not mentioned in 1.63.0 release notes #7090

Closed
pellared opened this issue Apr 4, 2024 · 7 comments · Fixed by #7103
Closed

Deprecation of DialContext and Dial is not mentioned in 1.63.0 release notes #7090

pellared opened this issue Apr 4, 2024 · 7 comments · Fixed by #7103

Comments

@pellared
Copy link
Contributor

pellared commented Apr 4, 2024

DialContext and ClientConn.Dial were deprecated by #7029 and it is not mentioned in 1.63.0 release notes.

@pellared pellared changed the title Deprecation not mentioned in 1.63.0 release notes Deprecation of DialContext and Dial is not mentioned in 1.63.0 release notes Apr 4, 2024
@aaronbee
Copy link

aaronbee commented Apr 8, 2024

I don't think it was a good idea to deprecate Dial and DialContext. They are not niche APIs. They are used by literally every grpc-go client implementation; you can't implement a gRPC client without it dialing a gRPC server. By marking them deprecated you are asking every user to evaluate every use of Dial, see how it should be updated, and submit a patch to make the change. This is not a trivial amount of work when you consider the wide use of gRPC.

There are 178,000 open source imports of grpc and countless more inside proprietary code bases and basically all of them are using Dial, either because they provide a grpc client, or a grpc server and they have end-to-end testing. For any of those projects using standard linting tools their CI's are blowing up with deprecation warnings. My one organization's codebase has over 300 calls to Dial, mostly in test cases.

What I would rather see is continued support for Dial with perhaps a doc comment on why a user might prefer to use NewClient instead.

@dfawley
Copy link
Member

dfawley commented Apr 8, 2024

https://go.dev/wiki/Deprecated

Sometimes an API feature such as a struct field, function, type, or even a whole package becomes redundant or unnecessary. When we want to discourage new programs from using it, we mark that feature “deprecated”.

In contrast to some other systems, an API feature being deprecated does not mean it is going to be removed in the future. On the contrary, Go 1 compatibility means the feature will be preserved in its deprecated form to keep existing programs running.

This all sounds WAI.

One thing we did get wrong according to this recommendation was to release NewClient at the same time as deprecating Dial/DialContext. We can un-deprecate for one release if it helps, but the decision is to call it deprecated to encourage the use of the new API.

@dfawley
Copy link
Member

dfawley commented Apr 8, 2024

What I would rather see is continued support for Dial

Dial support isn't going away, by the way. It will stick around.

Yes, there will be a linter warning if you run a linter that complains about your use of deprecated features. You should have a way to silence linters that you disagree with. That's a tooling problem, not a problem with our decision to deprecate this API. New users need to be funneled to NewClient instead, and Deprecated tags are how that is done.

@howardjohn
Copy link
Contributor

Seems like this was only fixed in 1.63 but not in the new 1.64.0?

Was the issue that was fixed that it was undocumented, and now that it was noted in the release note the problem is solved? IMO that is not the issue, the issue is the deprecation to begin with. As #7090 (comment) notes, this will require 178,000 usages to be changed across the ecosystem.

If this issue was supposed to be scope only to "undocumented" and not "do not deprecate it" let me know and I can open a new issue or PR.

@pellared
Copy link
Contributor Author

pellared commented May 15, 2024

@howardjohn, the issue was only about missing docs. You should create a new one (or find an existing one) if you want to ask for "undeprecating" the deprecated API.

@jon-whit
Copy link

Could the maintainer team as least suggest some replacements for usages of Dial(WithBlock()) or Dial(WithTimeout())? That's the main migration path that I see practically impacting most people affected by the deprecation cycle.

I'm not seeing a clear or elegant path to replacing that functionality with NewClient without adding a lot more boilerplate beyond what previously existed with Dial. Specifically, if a developer wants to wait for the connection state to be "Ready" and block until that is the case, what is the advisable way to achieve that with the new API patterns?

@dfawley
Copy link
Member

dfawley commented May 31, 2024

@jon-whit

Have you seen https://github.com/grpc/grpc-go/blob/master/Documentation/anti-patterns.md already?

You have a couple options for what you are asking for:

  1. Keep using grpc.Dial. If your staticcheck/etc is complaining about using something deprecated, disable it.
  2. Use grpc.NewClient followed by polling ClientConn.State() and WaitForStateChange(). https://pkg.go.dev/google.golang.org/grpc#ClientConn.WaitForStateChange This is what we do internally to support WithBlock and WithTimeout (made more complicated to handle other un-recommended options):

grpc-go/clientconn.go

Lines 266 to 290 in 24e9024

for {
s := cc.GetState()
if s == connectivity.Idle {
cc.Connect()
}
if s == connectivity.Ready {
return cc, nil
} else if cc.dopts.copts.FailOnNonTempDialError && s == connectivity.TransientFailure {
if err = cc.connectionError(); err != nil {
terr, ok := err.(interface {
Temporary() bool
})
if ok && !terr.Temporary() {
return nil, err
}
}
}
if !cc.WaitForStateChange(ctx, s) {
// ctx got timeout or canceled.
if err = cc.connectionError(); err != nil && cc.dopts.returnLastError {
return nil, err
}
return nil, ctx.Err()
}
}

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

Successfully merging a pull request may close this issue.

5 participants