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

Provide synchronous way of accessing application default credentials #652

Closed
jskeet opened this issue Jan 5, 2016 · 5 comments
Closed
Assignees
Milestone

Comments

@jskeet
Copy link
Collaborator

jskeet commented Jan 5, 2016

Currently we only have GoogleCredentials.GetApplicationDefaultAsync. That leaves callers unclear about how to fetch the default credentials in a synchronous piece of code without potentially deadlocking.

It looks like all we'd need to provide is a synchronous form of ComputeCredential.IsRunningOnComputeEngine()... that's the only thing awaited in DefaultCredentialProvider.CreateDefaultCredentialAsync().

@peleyal
Copy link
Collaborator

peleyal commented Jan 5, 2016

Good point.
In cases like this, I like to look what other client libraries do before deciding what is the best way to approach this one.
So, for example HttpClient (which I like) doesn't support Delete, Put, or other methods. It has only the Async version. Does it confuse users? I'm not sure at all...

In addition, I think that we do have a problem that we don't have standards.
Sometimes, our client library does support both Async and non-async version (like 'IClientServiceRequest', https://github.com/google/google-api-dotnet-client/blob/master/Src/GoogleApis/Apis/Requests/IClientServiceRequest.cs#L71), but sometimes it supports only the Async version, just like the class you mentioned or GoogleAuthorizationBroker, https://github.com/google/google-api-dotnet-client/blob/781bef1440362dd65333411ed6c5aff107b62f73/Src/GoogleApis.Auth.DotNet4/OAuth2/GoogleWebAuthorizationBroker.cs#L51)

I like standards, but at the same time, I think that having only the Async version is fine (both async and non-async methods were written long time ago, later methods were written only for an async mode). What are your thoughts?

Thanks!
Eyal

@jskeet
Copy link
Collaborator Author

jskeet commented Jan 5, 2016

Well, HttpClient is async-only for basically everything. I guess if you don't want to use asynchrony, you use WebClient, assuming you're on a platform that supports that. (If you're not, you're probably on a platform which is async-only anyway, effectively.)

I'm generally uncomfortable with forcing everything to be asynchronous. While async/await is great, not every app has to be asynchronous, and consuming an async-only API within a sync-based app provides a real conceptual speed-bump in my experience (and makes the caller worry about whether it's safe to just use .Result etc - which depends on the implementation).

@peleyal
Copy link
Collaborator

peleyal commented Jan 6, 2016

I'm not sure that if you don't use asynchrony environment you will still choose WebClient since HttpClient is much more clean and straight forward than the later, but that isn't the real issue here...

The user shouldn't worry about calling Result, if it doesn't work, we are doing something bad.
Anyway, the bottom line is that we can provide a sync version for our methods, it will might even make the user life easier 👍

@jskeet
Copy link
Collaborator Author

jskeet commented Jan 6, 2016

The user shouldn't worry about calling Result, if it doesn't work, we are doing something bad.

While that's true for our implementation, it's not generally true of Task<T> - which means it's entirely reasonable for the user to at least flag it as a cause for concern.

Glad you're generally on-board though. Will start having a look at how to best achieve it.

jskeet added a commit to jskeet/google-api-dotnet-client that referenced this issue Jan 18, 2016
This addresses issue googleapis#652.

The "remarks" comment is deliberately off-putting, in that if you're in a situation where you *can*
sensibly use the async call, you should do so - but as the rest of the client library allows synchronous
calls, it would be odd not to do so for initialization.

Alternatives to consider:
- Put synchronous code in DefaultCredentialProvider (no obvious benefit, as we'd still need to await an HttpClient eventually)
- Just document the "safety" of using the Result property in the async call
@chrisdunelm
Copy link
Contributor

I think this is generally a good idea for the reasons mentioned above. Will possibly duplicate/resurrect #662.

@chrisdunelm chrisdunelm added this to the v1.27 milestone Apr 24, 2017
chrisdunelm added a commit to chrisdunelm/google-api-dotnet-client that referenced this issue May 26, 2017
chrisdunelm added a commit to chrisdunelm/google-api-dotnet-client that referenced this issue May 27, 2017
chrisdunelm added a commit to chrisdunelm/google-api-dotnet-client that referenced this issue May 27, 2017
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

No branches or pull requests

3 participants