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
Fixes source token expiration by token refresh #5198
Changes from 1 commit
236446f
08f87c0
6409aba
0935361
af6a82d
f3b4729
101425e
303bc30
3c5f2a8
493e215
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,10 @@ | ||
import { logger } from "../../../logger"; | ||
import { Timer } from "./timer"; | ||
|
||
enum TokenState { | ||
"NONE", | ||
"VALID", | ||
"INVALID", | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tangent: enums are controversial in TS for some reason. https://blog.logrocket.com/why-typescript-enums-suck/ Internally, I think we prefer string literals:
|
||
|
||
/** | ||
* GCF v1 deploys support reusing a build between function deploys. | ||
|
@@ -8,40 +13,54 @@ import { Timer } from "./timer"; | |
*/ | ||
export class SourceTokenScraper { | ||
private tokenValidDurationMs; // in ms | ||
private tokenRefreshRequired = true; | ||
private tokenRefreshTimer: Timer | undefined; | ||
private resolve!: (token?: string) => void; | ||
private promise: Promise<string | undefined>; | ||
private expiration: bigint | undefined; | ||
private tokenState: TokenState; | ||
|
||
constructor(validDurationMs = 1500000) { | ||
this.tokenValidDurationMs = validDurationMs; | ||
this.promise = new Promise((resolve) => (this.resolve = resolve)); | ||
this.tokenState = TokenState.NONE; | ||
} | ||
|
||
// Token Promise will return undefined for the first caller | ||
// (because we presume it's this function's source token we'll scrape) | ||
// and then returns the promise generated from the first function's onCall | ||
tokenPromise(): Promise<string | undefined> { | ||
if (this.tokenRefreshRequired) { | ||
this.tokenRefreshRequired = false; | ||
this.tokenRefreshTimer = new Timer(this.tokenValidDurationMs); | ||
return Promise.resolve(undefined); | ||
async getToken(): Promise<string | undefined> { | ||
if (this.tokenState === TokenState.NONE) { | ||
this.tokenState = TokenState.INVALID; | ||
return undefined; | ||
} else if (this.tokenState === TokenState.INVALID) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found that token states as described here bit confusing. What about another name that closely maps to the state of the scraper, like |
||
return this.promise; // wait until we get a source token | ||
} else if (this.tokenState === TokenState.VALID) { | ||
if (this.checkTokenExpired()) { | ||
this.tokenState = TokenState.INVALID; | ||
this.promise = new Promise((resolve) => (this.resolve = resolve)); | ||
return undefined; | ||
} | ||
return this.promise; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we throw an error here instead of silently succeeding? Can see argument for either. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By throwing an error, do you mean throwing an error and then catching it in an enclosing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean that in theory we should never reach this codepath - fetchState can only be in one of three states, and we have if condition for all of them. So we could throw (which could happen if we misunderstood this code for somee reason) or just return the promise (which might leave the CLI in an undefined state). I have slight preference for the former. |
||
} | ||
return this.promise; | ||
} | ||
|
||
checkTokenExpired(): boolean { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit* I usually name my boolean function with "is" like |
||
if (this.expiration === undefined) { | ||
throw new Error(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This error is user visible - we should try our best to describe the situation and also call out what users should do when they see the issue. |
||
} | ||
return process.hrtime.bigint() >= this.expiration; | ||
} | ||
|
||
calculateTokenExpiry(): bigint { | ||
const now = process.hrtime.bigint(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I think using I'd suggest we stick to |
||
return now + BigInt(this.tokenValidDurationMs) * BigInt(1e6); | ||
} | ||
|
||
get poller() { | ||
return (op: any) => { | ||
if (this.tokenRefreshTimer?.expired()) { | ||
this.tokenRefreshRequired = true; | ||
this.resolve(); | ||
return; | ||
} | ||
if (op.metadata?.sourceToken || op.done) { | ||
const [, , , /* projects*/ /* project*/ /* regions*/ region] = | ||
op.metadata?.target?.split("/") || []; | ||
logger.debug(`Got source token ${op.metadata?.sourceToken} for region ${region as string}`); | ||
this.resolve(op.metadata?.sourceToken); | ||
this.tokenState = TokenState.VALID; | ||
this.expiration = this.calculateTokenExpiry(); // calculate expiration | ||
} | ||
}; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,22 +3,26 @@ import { expect } from "chai"; | |
import { SourceTokenScraper } from "../../../../deploy/functions/release/sourceTokenScraper"; | ||
|
||
describe("SourceTokenScraper", () => { | ||
it("immediately provides the first result", async () => { | ||
const timeout = (duration: number): Promise<void> => { | ||
return new Promise<void>((resolve) => setTimeout(resolve, duration)); | ||
}; | ||
|
||
it.only("immediately provides the first result", async () => { | ||
const scraper = new SourceTokenScraper(); | ||
await expect(scraper.tokenPromise()).to.eventually.be.undefined; | ||
await expect(scraper.getToken()).to.eventually.be.undefined; | ||
}); | ||
|
||
it("provides results after the first operation completes", async () => { | ||
const scraper = new SourceTokenScraper(); | ||
// First result comes right away; | ||
await expect(scraper.tokenPromise()).to.eventually.be.undefined; | ||
await expect(scraper.getToken()).to.eventually.be.undefined; | ||
|
||
let gotResult = false; | ||
const timeout = new Promise((resolve, reject) => { | ||
setTimeout(() => reject(new Error("Timeout")), 10); | ||
}); | ||
const getResult = (async () => { | ||
await scraper.tokenPromise(); | ||
await scraper.getToken(); | ||
gotResult = true; | ||
})(); | ||
await expect(Promise.race([getResult, timeout])).to.be.rejectedWith("Timeout"); | ||
|
@@ -28,48 +32,63 @@ describe("SourceTokenScraper", () => { | |
await expect(getResult).to.eventually.be.undefined; | ||
}); | ||
|
||
it("provides tokens from an operation", async () => { | ||
it.only("provides tokens from an operation", async () => { | ||
const scraper = new SourceTokenScraper(); | ||
// First result comes right away | ||
await expect(scraper.tokenPromise()).to.eventually.be.undefined; | ||
await expect(scraper.getToken()).to.eventually.be.undefined; | ||
|
||
scraper.poller({ | ||
metadata: { | ||
sourceToken: "magic token", | ||
target: "projects/p/locations/l/functions/f", | ||
}, | ||
}); | ||
await expect(scraper.tokenPromise()).to.eventually.equal("magic token"); | ||
await expect(scraper.getToken()).to.eventually.equal("magic token"); | ||
}); | ||
|
||
it.only("refreshes token after timer expires", async () => { | ||
const scraper = new SourceTokenScraper(100); // tokens expire every 100 ms | ||
// First result comes right away; | ||
await expect(scraper.tokenPromise()).to.eventually.be.undefined; | ||
|
||
const op = { | ||
const scraper = new SourceTokenScraper(10); | ||
await expect(scraper.getToken()).to.eventually.be.undefined; | ||
scraper.poller({ | ||
metadata: { | ||
sourceToken: "magic token", | ||
target: "projects/p/locations/l/functions/f", | ||
}, | ||
}; | ||
scraper.poller(op); | ||
}); | ||
await expect(scraper.getToken()).to.eventually.equal("magic token"); | ||
await timeout(50); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the future, try to instrument code without sleeping in your test. You can inject a clock for example. |
||
await expect(scraper.getToken()).to.eventually.be.undefined; | ||
scraper.poller({ | ||
metadata: { | ||
sourceToken: "magic token #2", | ||
target: "projects/p/locations/l/functions/f", | ||
}, | ||
}); | ||
await expect(scraper.getToken()).to.eventually.equal("magic token #2"); | ||
}); | ||
|
||
await expect(scraper.tokenPromise()).to.eventually.be.equal("magic token"); | ||
const timeout = (): Promise<void> => { | ||
return new Promise<void>((resolve) => setTimeout(resolve, 200)); | ||
}; | ||
await timeout(); | ||
scraper.poller(op); // sets token promise to undefined | ||
await expect(scraper.tokenPromise()).to.eventually.be.undefined; | ||
it.only("concurrent requests for source token", async () => { | ||
const scraper = new SourceTokenScraper(); | ||
|
||
// new token polled | ||
const promises = []; | ||
for (let i = 0; i < 3; i++) { | ||
promises.push(scraper.getToken()); | ||
} | ||
scraper.poller({ | ||
metadata: { | ||
sourceToken: "magic token 2", | ||
sourceToken: "magic token", | ||
target: "projects/p/locations/l/functions/f", | ||
}, | ||
}); | ||
await expect(scraper.tokenPromise()).to.eventually.be.equal("magic token 2"); | ||
|
||
let successes = 0; | ||
const tokens = await Promise.all(promises); | ||
for (const tok of tokens) { | ||
if (tok === "magic token") { | ||
successes++; | ||
} | ||
} | ||
expect(tokens.includes(undefined)).to.be.true; | ||
expect(successes).to.equal(2); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - stray comment?