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

Why does grpc.NewClient silently ignore DialOptions? #7328

Closed
SamMobed opened this issue Jun 17, 2024 · 2 comments
Closed

Why does grpc.NewClient silently ignore DialOptions? #7328

SamMobed opened this issue Jun 17, 2024 · 2 comments
Assignees

Comments

@SamMobed
Copy link

Hi,

The grpc.NewClient godoc specifies how DialOptions returned by WithBlock, WithTimeout, WithReturnConnectionError, and FailOnNonTempDialError are ignored by it.

In the following document, it's explained that using deprecated DialOptions can cause problems.

I'm wondering why NewClient silently ignores these options when they are set by users, instead of returning an error? The user might be expecting a certain behavior when creating the connection, and silently ignoring these options instead of explicitly raising an error might hurt their work.

@purnesh42H purnesh42H self-assigned this Jun 18, 2024
@zasweq zasweq assigned atollena and unassigned purnesh42H Jun 18, 2024
@atollena
Copy link
Collaborator

atollena commented Jun 19, 2024

I don't know if erroring has been considered and rejected, or if we just didn't think about it (the PR introducing NewClient has no mention of this option). I could see the argument for accepting those options to make it easier to switch to NewClient. I am not sure this was the best option: if users do rely on WithBlock, and since NewClient is a new function, I suppose that in most cases it's easy enough for users to get rid of those ineffective and deprecated options when they migrate to the new interface. And it makes it even more explicit that the blocking behaviour doesn't apply anymore.

Introducing this error as a breaking change might be OK. @arvindbr8 mentioned that issuing logs might be a reasonable trade-off. We'll wait for @dfawley to decide if we want to do something about it though, to see if those options were considered.

@dfawley
Copy link
Member

dfawley commented Jun 20, 2024

Since Dial calls NewClient, we'd need to filter these options out there before calling NewClient. I think the current behavior is fine (even if it's not optimal), and changing it now will be hard, since it would be a breaking behavior change. I'd be inclined to leave things as-is, personally.

@dfawley dfawley closed this as completed Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants