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

How to set a timeout for GoogleCredentials? #356

Open
hiranya911 opened this issue Oct 8, 2019 · 7 comments
Open

How to set a timeout for GoogleCredentials? #356

hiranya911 opened this issue Oct 8, 2019 · 7 comments
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@hiranya911
Copy link
Contributor

hiranya911 commented Oct 8, 2019

GoogleCredentials can be initialized with an optional HttpTransportFactory. But this doesn't provide any means of setting the connection and read timeout for the HTTP requests made by the credentials implementation. Is there a way to set these timeout values for GoogleCredentials?

More context: In the Google HTTP client, the recommended way to set timeouts is via an HttpRequestInitializer that gets passed into the HttpRequestFactory.

myTransport.createRequestFactory(new HttpRequestInitializer() {
      @Override
      public void initialize(HttpRequest request) throws IOException {
        request.setConnectTimeout(10000);
        request.setReadTimeout(10000);
      }
    });

But currently there appears to be no way to specify a request initializer for GoogleCredentials. As a result, there's no simple way to configure the timeouts for the HTTP requests made by this implementation.

This is a somewhat critical issue, since GoogleCredentials get invoked before any other HTTP requests are made by the client. The default timeout settings of GoogleCredentials essentially mask and override any timeout values set in the client.

myTransport.createRequestFactory(new HttpRequestInitializer() {
      @Override
      public void initialize(HttpRequest request) throws IOException {
        credentials.initialize(request);
        request.setConnectTimeout(10000);
        request.setReadTimeout(10000);
      }
    });

In the above example you expect requests to timeout after 10 seconds. But because credentials are in the initializer chain, it won't timeout until 60s (the default).

@hiranya911
Copy link
Contributor Author

There's also some retry logic baked into the flow that is not configurable:

HttpRequestFactory requestFactory = transportFactory.create().createRequestFactory();
HttpRequest request = requestFactory.buildPostRequest(new GenericUrl(tokenServerUri), content);
request.setParser(new JsonObjectParser(jsonFactory));
request.setIOExceptionHandler(new HttpBackOffIOExceptionHandler(new ExponentialBackOff()));
request.setUnsuccessfulResponseHandler(
new HttpBackOffUnsuccessfulResponseHandler(new ExponentialBackOff())
.setBackOffRequired(
new BackOffRequired() {
public boolean isRequired(HttpResponse response) {
int code = response.getStatusCode();
return (
// Server error --- includes timeout errors, which use 500 instead of 408
code / 100 == 5
// Forbidden error --- for historical reasons, used for rate_limit_exceeded
// errors instead of 429, but there currently seems no robust automatic way
// to
// distinguish these cases: see
// https://github.com/google/google-api-java-client/issues/662
|| code == 403);
}
}));

By default failing requests are retried up to 10 times with exponential backoff.

@yoshi-automation yoshi-automation added triage me I really want to be triaged. 🚨 This issue needs some love. labels Oct 9, 2019
@codyoss codyoss added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed 🚨 This issue needs some love. triage me I really want to be triaged. labels Oct 15, 2019
@codyoss
Copy link
Member

codyoss commented Oct 15, 2019

I agree, there does not seem to be a easy way to do this today. Is your use case you would like to fail faster in your code? Although it is not ideal, I bet you could add timeouts in by wrapping GoogleCredentials as not a lot seems to be final.

@hiranya911
Copy link
Contributor Author

Yes, fail fast would be one use case.

Although it is not ideal, I bet you could add timeouts in by wrapping GoogleCredentials as not a lot seems to be final.

Does that imply implementing some sort of a timer around the GoogleCredentials.refreshAccessToken() API?

It appears there's no easy way to implement actual connection/socket timeouts since HTTP request logic is essentially buried in the GoogleCredentials.refreshAccessToken() method. Couple of solutions I can think of:

  1. Make it possible to pass an HttpRequestInitializer into GoogleCredentials.
  2. Expose an initializeTokenRequest(HttpRequest req) method from GoogleCredentials that users can override. Default implementation will do nothing.

@codyoss
Copy link
Member

codyoss commented Oct 15, 2019

Does that imply implementing some sort of a timer around the GoogleCredentials.refreshAccessToken() API?

That method needs to be implemented, its default impl from the base class is to throw an exception. Are you using GoogleCredentials directly or one of the other classes that implement it?

I will bring those possible solutions up with my team. Thank you for your input!

@hiranya911
Copy link
Contributor Author

Ours is also a developer product: https://github.com/firebase/firebase-admin-java

Developers pass GoogleCredentials instances to our SDK. I believe most developers would use one of the factory methods to create credentials -- GoogleCredentials.fromStream(...). The actual instance they get is something like ServiceAccountCredentials or ApplicationDefaultCredentials.

It would be great if there was an API to wrap an existing GoogleCredentials instance with a timeout. This way our SDK can "inject" a timeout to the object passed in by the developer:

GoogleCredentials copy = credentials.withTimeout(connectTimeout, readTimeout);

// Or more generally
GoogleCredentials copy = credentials.withHttpRequestInitializer(initializer);

But we can also settle for a solution that would require developers to initialize GoogleCredentials with a timeout or a request initializer.

@anmol1vw13
Copy link

@hiranya911 @yoshi-automation Is there an update on this?

@stager0909
Copy link

#1053

please check merge request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

5 participants