From dcf91e1844fb0f5d70a4bbe57cfa2827a357a9d4 Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Thu, 20 Feb 2025 17:18:07 -0800 Subject: [PATCH 1/6] Improve robustness of function discovery for python Anecdotally, python function discovery is flakey. We propose 2 change in this PR: 1. For python discovery, add a small initial delay for python's admin server to boot. 2. Add a request timeout to retry call to retrieve trigger information. Previously, the default timeout would've been set to OS-level TCP timeout, which in my laptop was between 20~30s. --- .../functions/runtimes/discovery/index.spec.ts | 13 +++++++++++++ .../functions/runtimes/discovery/index.ts | 18 ++++++++++++++---- src/deploy/functions/runtimes/python/index.ts | 7 ++++++- 3 files changed, 33 insertions(+), 5 deletions(-) diff --git a/src/deploy/functions/runtimes/discovery/index.spec.ts b/src/deploy/functions/runtimes/discovery/index.spec.ts index d047f5fd886..205a0af28b1 100644 --- a/src/deploy/functions/runtimes/discovery/index.spec.ts +++ b/src/deploy/functions/runtimes/discovery/index.spec.ts @@ -101,9 +101,22 @@ describe("detectFromPort", () => { code: "ECONNREFUSED", }); + nock("http://127.0.0.1:8080").get("/__/functions.yaml").times(3).replyWithError({ + message: "Almost there", + code: "ETIMEDOUT", + }); + nock("http://127.0.0.1:8080").get("/__/functions.yaml").reply(200, YAML_TEXT); const parsed = await discovery.detectFromPort(8080, "project", "nodejs16"); expect(parsed).to.deep.equal(BUILD); }); + + it("retries when request times out", async () => { + nock("http://127.0.0.1:8081").get("/__/functions.yaml").delay(1_000).reply(200, YAML_TEXT); + nock("http://127.0.0.1:8080").get("/__/functions.yaml").reply(200, YAML_TEXT); + + const parsed = await discovery.detectFromPort(8080, "project", "nodejs16", 0, 500); + expect(parsed).to.deep.equal(BUILD); + }); }); diff --git a/src/deploy/functions/runtimes/discovery/index.ts b/src/deploy/functions/runtimes/discovery/index.ts index cc80b9889ed..afd59f6e618 100644 --- a/src/deploy/functions/runtimes/discovery/index.ts +++ b/src/deploy/functions/runtimes/discovery/index.ts @@ -75,7 +75,9 @@ export async function detectFromPort( port: number, project: string, runtime: Runtime, - timeout = 10_000 /* 10s to boot up */, + initialDelay = 0, + reqTimeout = 5_000 /* 5s per request timeout */, + timeout = 16_000 /* 16s global timeout */, ): Promise { let res: Response; const timedOut = new Promise((resolve, reject) => { @@ -84,13 +86,21 @@ export async function detectFromPort( }, getFunctionDiscoveryTimeout() || timeout); }); + // Initial delay to wait for admin server to boot. + if (initialDelay > 0) { + await new Promise((resolve) => setTimeout(resolve, initialDelay)); + } + + const url = `http://127.0.0.1:${port}/__/functions.yaml`; while (true) { try { - res = await Promise.race([fetch(`http://127.0.0.1:${port}/__/functions.yaml`), timedOut]); + res = await Promise.race([fetch(url, { timeout: reqTimeout }), timedOut]); break; } catch (err: any) { - // Allow us to wait until the server is listening. - if (err?.code === "ECONNREFUSED") { + if ( + err?.name === "FetchError" || + ["ECONNREFUSED", "ECONNRESET", "ETIMEDOUT"].includes(err?.code) + ) { continue; } throw err; diff --git a/src/deploy/functions/runtimes/python/index.ts b/src/deploy/functions/runtimes/python/index.ts index 1ffcc93fea0..1bcf0796103 100644 --- a/src/deploy/functions/runtimes/python/index.ts +++ b/src/deploy/functions/runtimes/python/index.ts @@ -195,7 +195,12 @@ export class Delegate implements runtimes.RuntimeDelegate { }); const killProcess = await this.serveAdmin(adminPort, envs); try { - discovered = await discovery.detectFromPort(adminPort, this.projectId, this.runtime); + discovered = await discovery.detectFromPort( + adminPort, + this.projectId, + this.runtime, + 500 /* initialDelay, python startup is slow */, + ); } finally { await killProcess(); } From a445c13d9fd2688dee8ced94ac3224ea191cef77 Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Thu, 20 Feb 2025 17:21:39 -0800 Subject: [PATCH 2/6] Add changelog. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e69de29bb2d..a477d70450d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -0,0 +1 @@ +- Improve robustness of function discovery for python (#8239) From 6591842dc9219e49775c27d6f4172c0c12ea4d7a Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Fri, 21 Feb 2025 14:12:02 -0800 Subject: [PATCH 3/6] Remove per-req timeout to accomodate loading large/slow main.py. --- src/deploy/functions/runtimes/discovery/index.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/deploy/functions/runtimes/discovery/index.ts b/src/deploy/functions/runtimes/discovery/index.ts index afd59f6e618..b1082dd4f19 100644 --- a/src/deploy/functions/runtimes/discovery/index.ts +++ b/src/deploy/functions/runtimes/discovery/index.ts @@ -76,8 +76,7 @@ export async function detectFromPort( project: string, runtime: Runtime, initialDelay = 0, - reqTimeout = 5_000 /* 5s per request timeout */, - timeout = 16_000 /* 16s global timeout */, + timeout = 15_000 /* 15s global timeout */, ): Promise { let res: Response; const timedOut = new Promise((resolve, reject) => { @@ -94,7 +93,7 @@ export async function detectFromPort( const url = `http://127.0.0.1:${port}/__/functions.yaml`; while (true) { try { - res = await Promise.race([fetch(url, { timeout: reqTimeout }), timedOut]); + res = await Promise.race([fetch(url), timedOut]); break; } catch (err: any) { if ( @@ -112,7 +111,7 @@ export async function detectFromPort( logger.debug(`Got response code ${res.status}; body ${text}`); throw new FirebaseError( "Functions codebase could not be analyzed successfully. " + - "It may have a syntax or runtime error", + "It may have a syntax or runtime error", ); } const text = await res.text(); From cf2ee3f0c864cd3d0ced2591a9846d6e2af8d042 Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Fri, 21 Feb 2025 14:29:13 -0800 Subject: [PATCH 4/6] Update changelog. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e659c90097..697303b2844 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,2 +1,2 @@ -- Improve robustness of function discovery for python (#8239) +- Add initial delay when loading python functions (#8239) - Enforce webframeworks enablement only on webframeworks sites (#8168) From 39224fab31f300c655a4b731bdc6daf0589e5d51 Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Fri, 21 Feb 2025 14:31:25 -0800 Subject: [PATCH 5/6] Revert timeout bump. --- src/deploy/functions/runtimes/discovery/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/deploy/functions/runtimes/discovery/index.ts b/src/deploy/functions/runtimes/discovery/index.ts index b1082dd4f19..cf6f0d5ba0a 100644 --- a/src/deploy/functions/runtimes/discovery/index.ts +++ b/src/deploy/functions/runtimes/discovery/index.ts @@ -76,7 +76,7 @@ export async function detectFromPort( project: string, runtime: Runtime, initialDelay = 0, - timeout = 15_000 /* 15s global timeout */, + timeout = 10_000 /* 10s to boot up */, ): Promise { let res: Response; const timedOut = new Promise((resolve, reject) => { From dcc15fd09f3f1ea711104e1978a4b0259943c29c Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Fri, 21 Feb 2025 14:37:06 -0800 Subject: [PATCH 6/6] Linting. --- src/deploy/functions/runtimes/discovery/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/deploy/functions/runtimes/discovery/index.ts b/src/deploy/functions/runtimes/discovery/index.ts index cf6f0d5ba0a..c89c3a40f0b 100644 --- a/src/deploy/functions/runtimes/discovery/index.ts +++ b/src/deploy/functions/runtimes/discovery/index.ts @@ -111,7 +111,7 @@ export async function detectFromPort( logger.debug(`Got response code ${res.status}; body ${text}`); throw new FirebaseError( "Functions codebase could not be analyzed successfully. " + - "It may have a syntax or runtime error", + "It may have a syntax or runtime error", ); } const text = await res.text();