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

all: support gRPC dial timeout #946

Closed
jba opened this issue Mar 27, 2018 · 4 comments
Closed

all: support gRPC dial timeout #946

jba opened this issue Mar 27, 2018 · 4 comments
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release.

Comments

@jba
Copy link
Contributor

jba commented Mar 27, 2018

It should be possible to express a timeout on connection dial by writing

ctx = context.WithTimeout(ctx, 10*time.Second)
c, err := pkg.NewClient(ctx, ...)

The problem is that the same context passed to NewClient is used in constructing a TokenSource, which will repeatedly try to renew creds forever. We'd like to ignore deadlines and cancelation on that TokenSource context.

@jba jba added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Mar 27, 2018
@jba jba self-assigned this Mar 27, 2018
@jba
Copy link
Contributor Author

jba commented Mar 27, 2018

@zombiezen Could we use a detachable context for this? Is one available?

@zombiezen
Copy link
Contributor

You don't really want this. gRPC client conns shouldn't block on healthy, they should block on first RPC. That subsequent RPC should have a deadline set, which is the thing you would care about.

@lhecker
Copy link
Contributor

lhecker commented Mar 27, 2018

Thanks @zombiezen! It makes sense when you consider the possibility of lazy connections.
In my code I often rely on the above, requested feature to make instances fail early on startup if connections cannot be created at all.
I'm fixing my code now wherever I can find this mistake. This could possibly replaced with a simple SELECT 1 query I guess?

And maybe this too could be added one day to the documentation. 🙂
I have to say though, that it's a bit weird to be able to pass a context into the function, because it doesn't do what you expect it to do. And that's probably a bit inoptimal API-wise. 😕

@jba jba added documentation and removed type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Mar 28, 2018
@jba
Copy link
Contributor Author

jba commented Mar 28, 2018

Agreed. We will improve our docs on how to use context.

gcf-owl-bot bot added a commit that referenced this issue Oct 23, 2024
Remove the gRPC binding for generated compute protos as they
are not valid. This reduces the LoC in this package by ~13%.
Source-Link: googleapis/googleapis@80bfb11

Source-Link: googleapis/googleapis-gen@1096119
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiMTA5NjExOTczYWVlN2Y0MDFkYzdkNzExNWUwZDYwYjU0ZDdkYmNkMSJ9
gcf-merge-on-green bot pushed a commit that referenced this issue Oct 24, 2024
#11024)

- [ ] Regenerate this pull request now.

PiperOrigin-RevId: 689439500

Source-Link: https://togithub.com/googleapis/googleapis/commit/9e7a2e530599ebb1d0ed0ffb298b716585a366fe

Source-Link: https://togithub.com/googleapis/googleapis-gen/commit/069e663b58bb092e6447d2073373bc56517abc4c
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiMDY5ZTY2M2I1OGJiMDkyZTY0NDdkMjA3MzM3M2JjNTY1MTdhYmM0YyJ9
BEGIN_NESTED_COMMIT
feat(spanner/admin/instance): Add support for Cloud Spanner Default Backup Schedules
PiperOrigin-RevId: 688946300

Source-Link: https://togithub.com/googleapis/googleapis/commit/b11e6b0741fc333f7d558447f2efda76db44243d

Source-Link: https://togithub.com/googleapis/googleapis-gen/commit/f93f56b21ff01e499977c4dd54689cce1b7cf530
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZjkzZjU2YjIxZmYwMWU0OTk5NzdjNGRkNTQ2ODljY2UxYjdjZjUzMCJ9
END_NESTED_COMMIT
BEGIN_NESTED_COMMIT
feat(shopping/css): A new field `headline_offer_installment` is added to message `.google.shopping.css.v1.Attributes`
feat(shopping/css): A new field `headline_offer_subscription_cost` is added to message `.google.shopping.css.v1.Attributes`
feat(shopping/css): A new message `HeadlineOfferSubscriptionCost` is added
feat(shopping/css): A new message `HeadlineOfferInstallment` is added
feat(shopping/css): A new enum `SubscriptionPeriod` is added

PiperOrigin-RevId: 688649184

Source-Link: https://togithub.com/googleapis/googleapis/commit/c8a726182c087afc514aa51a980b937d07f86540

Source-Link: https://togithub.com/googleapis/googleapis-gen/commit/3dcef19e307b679a3f6c881d79f064e885fd52fd
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiM2RjZWYxOWUzMDdiNjc5YTNmNmM4ODFkNzlmMDY0ZTg4NWZkNTJmZCJ9
END_NESTED_COMMIT
BEGIN_NESTED_COMMIT
fix(compute): reduce size of Go compute package (#946)
Remove the gRPC binding for generated compute protos as they
are not valid. This reduces the LoC in this package by ~13%.
Source-Link: https://togithub.com/googleapis/googleapis/commit/80bfb115ac768be8fc3982123ab9fe11c3c5439e

Source-Link: https://togithub.com/googleapis/googleapis-gen/commit/109611973aee7f401dc7d7115e0d60b54d7dbcd1
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiMTA5NjExOTczYWVlN2Y0MDFkYzdkNzExNWUwZDYwYjU0ZDdkYmNkMSJ9
END_NESTED_COMMIT
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release.
Projects
None yet
Development

No branches or pull requests

3 participants