Skip to content
This repository was archived by the owner on Nov 20, 2025. It is now read-only.

fix: cache promise instead of ProjectId#216

Merged
JustinBeckwith merged 2 commits intogoogleapis:nextfrom
JustinBeckwith:187-2
Jan 8, 2018
Merged

fix: cache promise instead of ProjectId#216
JustinBeckwith merged 2 commits intogoogleapis:nextfrom
JustinBeckwith:187-2

Conversation

@JustinBeckwith
Copy link
Copy Markdown
Contributor

Addresses #187

@JustinBeckwith JustinBeckwith requested review from a team and ofrobots December 23, 2017 04:59
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 23, 2017
Copy link
Copy Markdown
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This harmless looking change will break context propagation. For some background see this code and the corresponding issue in google-auto-auth.

Speaking of which; google-auto-auth already caches the auth client and project id. Such logic / automation belongs in that package, not here.

@JustinBeckwith
Copy link
Copy Markdown
Contributor Author

The way this code works today, it can be kind of a bad bug. Take a look at #187 - we're queuing up thousands of calls, waiting for the initial projectId to populate. Generally - I'd say it's more important to fix the bug than to make it work with Stackdriver (totally willing to debate it of course).

On the discussion of this library vs google-auto-auth - I view that library as a band aid, because this one was basically abandoned. I would like to reach a place where it isn't needed anymore, but it is going to take time.

If we wanted to cache the promise, and not break context propagation - what can I do?

@ofrobots
Copy link
Copy Markdown
Contributor

Generally - I'd say it's more important to fix the bug than to make it work with Stackdriver

It is not about Stackdriver. This is about breaking long stack traces, NewRelic and all other async debugging tools out this. This module is the basis of all @google-cloud/ modules which means that all of them will stop working with these debug / tracing tools.

If we wanted to cache the promise, and not break context propagation - what can I do?

Similar to what was done in stephenplusplus/google-auto-auth#21. Basically, use the promise as a queue while it is pending. Once resolved, cache the value too, and stop using the promise.

@JustinBeckwith
Copy link
Copy Markdown
Contributor Author

I think I got it :) @ofrobots can you take another look?

@JustinBeckwith JustinBeckwith merged this pull request into googleapis:next Jan 8, 2018
ofrobots pushed a commit that referenced this pull request Jan 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants