Add lifecycle hooks and setImmediate polyfill for cf workers #1447
Conversation
WalkthroughAdds Cloudflare Worker execution-context support and an auto-detecting setImmediate, updates beforeCompile to polyfill and attach it, overrides fetch to propagate ExecutionContext, compiles routes and triggers onStart, adds stop to run onStop, and changes afterResponse scheduling to prefer ctx.waitUntil then setImmediate then Promise. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Adapter as CloudflareAdapter
participant Global as global/globalThis
participant Routes as Routes Compiler
Note over Adapter,Global: beforeCompile flow
App->>Adapter: beforeCompile(app)
Adapter->>Global: createAutoDetectingSetImmediate() -> attach to global
Adapter->>Adapter: override fetch to capture/propagate ExecutionContext
Adapter->>Routes: compile routes
Adapter-->>App: invoke onStart hooks
sequenceDiagram
autonumber
participant Handler
participant EnvDetector as Environment
participant Worker as Cloudflare Worker
participant Node as Node.js
participant Micro as Microtask (Promise)
Note over Handler,EnvDetector: afterResponse scheduling
Handler->>EnvDetector: schedule runAfterResponse(requestContext?)
alt ExecutionContext available
EnvDetector->>Worker: waitUntil(runAfterResponse)
else setImmediate available
EnvDetector->>Node: setImmediate(runAfterResponse)
else
EnvDetector->>Micro: Promise.resolve().then(runAfterResponse)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/adapter/cloudflare-worker/index.ts(2 hunks)src/dynamic-handle.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/dynamic-handle.ts (1)
test/types/index.ts (2)
afterResponse(1999-2009)afterResponse(2473-2475)
src/adapter/cloudflare-worker/index.ts (1)
src/adapter/web-standard/index.ts (1)
WebStandardAdapter(10-179)
|
@coderabbitai review this |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7192 @Vicentesan, here's a comprehensive review of your changes: 🎯 Overall AssessmentGood initiative to add Cloudflare Workers lifecycle support! However, there are several architectural and implementation concerns that should be addressed before merging. 🔴 Critical Issues1. Global State Management & Memory LeaksLocation: The
Recommendation: Clear the context after use: app.fetch = function (request: Request, env?: any, ctx?: ExecutionContext) {
try {
if (ctx) {
globalThis.__cloudflareExecutionContext = ctx
}
return originalFetch(request)
} finally {
// Clean up to prevent memory leaks and context bleeding
globalThis.__cloudflareExecutionContext = undefined
}
}2. Type Safety ViolationsLocation: Multiple The
Recommendation: Use proper type declarations or augment global types: declare global {
var setImmediate: ((callback: () => void) => void) | undefined
}3. Async/Sync Inconsistency in Lifecycle HooksLocation: The // Line 151-154 - NOT awaited
if (app.event.start)
for (let i = 0; i < app.event.start.length; i++)
app.event.start[i].fn(app) // ❌ What if this is async?Recommendation: Make async beforeCompile(app) {
// ... setup code ...
if (app.event.start)
for (let i = 0; i < app.event.start.length; i++)
await app.event.start[i].fn(app)
}
|
|
@coderabbitai review this |
|
@Vicentesan, I'll trigger a fresh review of the PR for you. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/adapter/cloudflare-worker/index.ts(3 hunks)src/adapter/types.ts(1 hunks)src/dynamic-handle.ts(4 hunks)
| // Override app.fetch to accept ExecutionContext and set it globally | ||
| const originalFetch = app.fetch.bind(app) | ||
| app.fetch = function ( | ||
| request: Request, | ||
| env?: any, | ||
| ctx?: ExecutionContext | ||
| ) { | ||
| try { | ||
| if (ctx) { | ||
| globalThis.__cloudflareExecutionContext = ctx | ||
| } | ||
| const result = originalFetch(request) | ||
| // Clean up context after request to prevent memory leaks and context bleeding | ||
| globalThis.__cloudflareExecutionContext = undefined | ||
| return result | ||
| } catch (error) { |
There was a problem hiding this comment.
Keep ExecutionContext alive until the request truly finishes
We set globalThis.__cloudflareExecutionContext = ctx, call originalFetch(request), and then immediately clear the global before the returned promise settles. All downstream async work (handlers, finally blocks, afterResponse, etc.) therefore run with the context wiped out, so waitUntil is never available and the feature still falls back to the Promise branch. The same happens on errors—cleanup runs before hooks fire.
Wrap the originalFetch call so the context remains set for the entire lifetime of the request and only restore/clear it in a finally after the promise resolves, e.g.:
- const result = originalFetch(request)
- // Clean up context after request to prevent memory leaks and context bleeding
- globalThis.__cloudflareExecutionContext = undefined
- return result
+ const previous = globalThis.__cloudflareExecutionContext
+ if (ctx) globalThis.__cloudflareExecutionContext = ctx
+
+ const result = originalFetch(request)
+
+ const cleanup = () => {
+ if (previous === undefined) {
+ delete globalThis.__cloudflareExecutionContext
+ } else {
+ globalThis.__cloudflareExecutionContext = previous
+ }
+ }
+
+ return result instanceof Promise ? result.finally(cleanup) : (cleanup(), result)This keeps waitUntil reachable for afterResponse while still preventing leaks and restores any prior context to avoid cross-request bleeding.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Override app.fetch to accept ExecutionContext and set it globally | |
| const originalFetch = app.fetch.bind(app) | |
| app.fetch = function ( | |
| request: Request, | |
| env?: any, | |
| ctx?: ExecutionContext | |
| ) { | |
| try { | |
| if (ctx) { | |
| globalThis.__cloudflareExecutionContext = ctx | |
| } | |
| const result = originalFetch(request) | |
| // Clean up context after request to prevent memory leaks and context bleeding | |
| globalThis.__cloudflareExecutionContext = undefined | |
| return result | |
| } catch (error) { | |
| // Override app.fetch to accept ExecutionContext and set it globally | |
| const originalFetch = app.fetch.bind(app) | |
| app.fetch = function ( | |
| request: Request, | |
| env?: any, | |
| ctx?: ExecutionContext | |
| ) { | |
| try { | |
| if (ctx) { | |
| globalThis.__cloudflareExecutionContext = ctx | |
| } | |
| const previous = globalThis.__cloudflareExecutionContext | |
| if (ctx) globalThis.__cloudflareExecutionContext = ctx | |
| const result = originalFetch(request) | |
| const cleanup = () => { | |
| if (previous === undefined) { | |
| delete globalThis.__cloudflareExecutionContext | |
| } else { | |
| globalThis.__cloudflareExecutionContext = previous | |
| } | |
| } | |
| return result instanceof Promise | |
| ? result.finally(cleanup) | |
| : (cleanup(), result) | |
| } catch (error) { | |
| // existing error handling... | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/adapter/cloudflare-worker/index.ts around lines 131 to 146, the code sets
globalThis.__cloudflareExecutionContext before calling originalFetch(request)
but clears it immediately after the call, which tears down the context before
the returned promise settles; change this so the context remains set for the
full lifetime of the request by saving the previous context, assigning
globalThis.__cloudflareExecutionContext = ctx, calling originalFetch(request)
and then attaching a finally handler (or awaiting the promise) that restores the
previous context (or sets undefined if none) only after the promise resolves or
rejects; ensure errors are re-thrown/passed through so behavior is unchanged and
memory/context leaks are prevented.
1 participant
addreses #1383
This PR enhances CF Workers support in Elysia, with a particular focus on lifecycle hooks and the
setImmediatepolyfillSummary by CodeRabbit
New Features
Bug Fixes