Skip to content

fix(auth): restore use of grpc.Dial#11118

Merged
gcf-merge-on-green[bot] merged 2 commits into
googleapis:mainfrom
codyoss:grpc
Nov 12, 2024
Merged

fix(auth): restore use of grpc.Dial#11118
gcf-merge-on-green[bot] merged 2 commits into
googleapis:mainfrom
codyoss:grpc

Conversation

@codyoss

@codyoss codyoss commented Nov 12, 2024

Copy link
Copy Markdown
Member

There have been a couple of reports lately around some subtle changes in behaviour of using the new recommend method. There is also a known bug, see grpc/grpc-go#7556. Until the bug is fixed and we have a way forward we will revert to calling the deprecated Dial function.

Fixes: #10780

There have been a couple of reports lately around some subtle
changes in behaviour of using the new recommend method. There is
also a known bug, see grpc/grpc-go#7556.
Until the bug is fixed and we have a way forward we will revert to
calling the deprecated Dial function.

Fixes: googleapis#7556
@codyoss codyoss requested a review from a team November 12, 2024 20:25
@quartzmo

Copy link
Copy Markdown
Member

Note that this reverts changes in #10780.

@codyoss codyoss added the automerge Merge the pull request once unit tests and other checks pass. label Nov 12, 2024
@gcf-merge-on-green gcf-merge-on-green Bot merged commit 2456b94 into googleapis:main Nov 12, 2024
@gcf-merge-on-green gcf-merge-on-green Bot removed the automerge Merge the pull request once unit tests and other checks pass. label Nov 12, 2024
@codyoss codyoss deleted the grpc branch November 12, 2024 20:45
tritone added a commit to tritone/google-cloud-go that referenced this pull request Nov 14, 2024
This picks up googleapis#11118 which should fix dial hangs for storage gRPC
methods.
tritone added a commit that referenced this pull request Nov 14, 2024
This picks up #11118 which should fix dial hangs for storage gRPC
methods.
@iwinux

iwinux commented Mar 31, 2025

Copy link
Copy Markdown

@quartzmo Previously auth/grpctransport/grpctransport.go was using grpc.DialContext. Why did you change it to grpc.Dial?

@codyoss

codyoss commented Mar 31, 2025

Copy link
Copy Markdown
Member Author

@iwinux See the linked grpc bug in the OP for details.

@iwinux

iwinux commented Mar 31, 2025

Copy link
Copy Markdown

I see no discussion related to Dial (the "restored" call) v.s. DialContext (the original call).

@codyoss

codyoss commented Mar 31, 2025

Copy link
Copy Markdown
Member Author

Oh sorry, I misread your question. I think maybe this should be DialContext, you are correct. Ill send a CL.

@codyoss

codyoss commented Mar 31, 2025

Copy link
Copy Markdown
Member Author

Ah looking closer into the gRPC code this was correct at the time to avoid that linked bug. DialContext calls NewClient and had this bug. This does appear to be fixed now though, so still proceeding.

codyoss added a commit to codyoss/google-cloud-go that referenced this pull request Mar 31, 2025
We switched to `Dial` orginally because of grpc/grpc-go#7556. Now
that this is fixed we can call DialContext again. In the future
we still will want to migrate to the new API directly, but this will
require more testing.

Related: googleapis#11118
gcf-merge-on-green Bot pushed a commit that referenced this pull request Mar 31, 2025
We switched to `Dial` orginally because of grpc/grpc-go#7556. Now that this is fixed we can call DialContext again. In the future we still will want to migrate to the new API directly, but this will require more testing.

Related: #11118
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants