Skip to content

Using a default HTTP timeout in db module #395

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

Closed
wants to merge 3 commits into from
Closed

Conversation

hiranya911
Copy link
Contributor

The google-auth package now sets a default timeout of 120 seconds for all outgoing requests (see googleapis/google-auth-library-python#435). This is causing a couple of test failures in our codebase: https://github.com/firebase/firebase-admin-python/actions/runs/32412728

But more importantly this change leads to unpredictable default behavior in applications that use firebase_admin. Specifically, developers who are on older versions of google-auth won't get any timeout (timeout=None), while developers on new versions will get a 2 minutes timeout (timeout=120).

This PR addresses the above discrepancy by setting a default timeout in our own code. This has the following advantages:

  1. Developers using firebase_admin will see consistent behavior regardless of their google-auth version.
  2. We get more control over the default timeout, and are no longer affected by the changes in google-auth.

Note that httpTimeout option is currently only respected by a subset of our modules. Other modules (e.g. auth), will get their default timeout from google-auth for now. I'm using the same default value of 120 seconds to mitigate this, but we should properly address this for all modules in a subsequent PR.

@hiranya911
Copy link
Contributor Author

Closing in favor of #397

@hiranya911 hiranya911 closed this Jan 29, 2020
@hiranya911 hiranya911 deleted the hkj-http-timeout branch January 29, 2020 19:58
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.

1 participant