-
-
Notifications
You must be signed in to change notification settings - Fork 409
Fix generators, use text/event-stream protocol
#743
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
Changes from 11 commits
95447b4
e078de0
303f544
88340ef
3b4fe64
e929003
742403c
cf7c2cb
683168f
4c1625b
19f92c5
823b136
e20d5d4
c804775
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 |
|---|---|---|
|
|
@@ -117,18 +117,24 @@ export const serializeCookie = (cookies: Context['set']['cookie']) => { | |
|
|
||
| // return arr | ||
| // } | ||
|
|
||
| const handleStream = async ( | ||
| generator: Generator | AsyncGenerator, | ||
| set?: Context['set'], | ||
| request?: Request | ||
| ) => { | ||
| let init = generator.next() | ||
| if (init instanceof Promise) init = await init | ||
|
|
||
| if (init.done) { | ||
| if (set) return mapResponse(init.value, set, request) | ||
| return mapCompactResponse(init.value, request) | ||
| let init | ||
| try { | ||
| init = generator.next() | ||
| if (init instanceof Promise) init = await init | ||
|
|
||
| if (init?.done) { | ||
| if (set) return mapResponse(init.value, set, request) | ||
| return mapCompactResponse(init.value, request) | ||
| } | ||
| } catch (error) { | ||
| // TODO should call app.onError if set | ||
| if (set) return mapResponse(error, set, request) | ||
| return mapCompactResponse(error, request) | ||
| } | ||
|
|
||
| return new Response( | ||
|
|
@@ -146,38 +152,29 @@ const handleStream = async ( | |
| } | ||
| }) | ||
|
|
||
| if (init.value !== undefined && init.value !== null) { | ||
| if ( | ||
| typeof init.value === "object" | ||
| if (init?.value !== undefined && init?.value !== null) | ||
| controller.enqueue( | ||
| Buffer.from( | ||
| `event: message\ndata: ${JSON.stringify(init.value)}\n\n` | ||
|
Member
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. Would make sense to check typeof object before using JSON.stringify
Author
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. If you don't always call JSON.stringify and the user yields a string that contains 2 new lines the response will break the text/event-stream protocol. If you always call JSON.stringify you can be sure that all the new lines are encoded with \n and put in a single line. The parsing in Eden also becomes simpler because you know messages are always a JSON encoded string instead of using heuristics. |
||
| ) | ||
| ) | ||
| try { | ||
| controller.enqueue( | ||
| Buffer.from(JSON.stringify(init.value)) | ||
| ) | ||
| } catch { | ||
| controller.enqueue(Buffer.from(init.value.toString())) | ||
| } | ||
| else controller.enqueue(Buffer.from(init.value.toString())) | ||
| } | ||
|
|
||
| for await (const chunk of generator) { | ||
| if (end) break | ||
| if (chunk === undefined || chunk === null) continue | ||
| try { | ||
| for await (const chunk of generator) { | ||
| if (end) break | ||
| if (chunk === undefined || chunk === null) continue | ||
|
|
||
| if (typeof chunk === 'object') | ||
| try { | ||
| controller.enqueue( | ||
| Buffer.from(JSON.stringify(chunk)) | ||
| controller.enqueue( | ||
| Buffer.from( | ||
| `event: message\ndata: ${JSON.stringify(chunk)}\n\n` | ||
remorses marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ) | ||
| } catch { | ||
| controller.enqueue(Buffer.from(chunk.toString())) | ||
| } | ||
| else controller.enqueue(Buffer.from(chunk.toString())) | ||
|
|
||
| // Wait for the next event loop | ||
| // Otherwise the data will be mixed up | ||
| await new Promise<void>((resolve) => | ||
| setTimeout(() => resolve(), 0) | ||
| ) | ||
| } | ||
| } catch (error: any) { | ||
| controller.enqueue( | ||
| Buffer.from( | ||
| `event: error\ndata: ${JSON.stringify(error.message || error.name || 'Error')}\n\n` | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
|
|
||
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.
You can remove try catch in here,
handleStreamfunction is already wrapped in try-catch that handle onError alreadyThere 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.
Currently the app will crash if you throw an error in a generator before yield, i added a test that reproduces this here: https://github.com/remorses/elysia/blob/932951679c89aef6bad08779b6eb7fe94c30335d/test/response/stream.test.ts#L75
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.
I fixed this crash removing the try catch when aot is false. In aot mode it still crashes.
the app crashes because the error is throws asynchronously while mapResponse is a not async, i think there are many more of these bugs hidden in the code, the ideal solution would be to always await
mapResonsein aot too