-
-
Notifications
You must be signed in to change notification settings - Fork 459
fix: preserve headers when throwing from AsyncGenerator #1679
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
base: main
Are you sure you want to change the base?
fix: preserve headers when throwing from AsyncGenerator #1679
Conversation
commit: |
WalkthroughExtended async detection to include AsyncGeneratorFunction and conditionally await Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant compose.ts
participant mapResponse
participant handleStream
participant onErrorHook
Client->>Handler: send request
Handler->>compose.ts: produce response (maybe async generator)
compose.ts->>mapResponse: build response mapping (maybeAsync + maybeStream)
alt streaming + async
compose.ts->>mapResponse: await mapResponse(...)
mapResponse->>handleStream: return stream proxy
handleStream->>compose.ts: error thrown in stream
compose.ts->>onErrorHook: invoke onError with error + headers
onErrorHook->>Client: respond with error status & headers
else non-stream or sync
compose.ts->>mapResponse: mapResponse(...) (no await)
mapResponse->>Client: send response
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🪛 Biome (2.1.2)test/response/stream.test.ts[error] 572-572: Unexpected constant condition. (lint/correctness/noConstantCondition) [error] 597-597: Unexpected constant condition. (lint/correctness/noConstantCondition) 🔇 Additional comments (3)
✏️ Tip: You can disable this entire section by setting 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 |
c74e6cb to
42aa87e
Compare
When throwing from an AsyncGenerator handler, headers set via set.headers were not being included in the error response. This was due to two issues: 1. AsyncGeneratorFunction was not being detected as async by isAsync(), causing the generated handler to be synchronous. 2. mapResponse returns a Promise when handling generators (via handleStream), but this Promise was not being awaited. When the generator threw, the Promise rejection escaped the try-catch block. The fix: - Updated isAsync() to recognize AsyncGeneratorFunction as async - Added 'await' to mapResponse when maybeStream && maybeAsync is true, ensuring Promise rejections from handleStream are properly caught and routed through error handling with preserved headers This ensures CORS headers and other custom headers work correctly when using throw inside async generator handlers. Closes elysiajs#1677
42aa87e to
1802497
Compare
Summary
Fixes #1677
When throwing from an AsyncGenerator handler, headers set via
set.headerswere not being included in the error response. This broke the CORS plugin and any other use case relying on custom headers in error responses from streaming handlers.Root Cause
The issue had two parts:
isAsync()didn't recognizeAsyncGeneratorFunction: The function only checked forAsyncFunction, causing async generator handlers to be treated as synchronous. This meant the generated handler function was notasync.mapResponsePromise not awaited: When handling generators,mapResponsecallshandleStreamwhich is async and returns a Promise. This Promise was returned directly withoutawait, so when the generator threw, the Promise rejection escaped the try-catch block instead of being caught and routed through error handling.Fix
Updated
isAsync()to also check forfn.constructor.name === 'AsyncGeneratorFunction'In the generated handler code, when
maybeStream && maybeAsyncis true, themapResponsecall is now awaited:Test Cases Added
should preserve headers when throwing from async generatorshould call onError hook when throwing from async generatorVerification
Before fix:
After fix:
onErrorhook is calledAll 1448 tests pass.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.