hono
hono copied to clipboard
honojs
Throwing (unhandled) exception in streamSSE brings down server
What version of Hono are you using?
3.12.11
What runtime/platform is your app running on?
Node
What steps can reproduce the bug?
app.get("/sse-error", (c) =>
streamSSE(c, async (stream) => {
stream.writeSSE({ data: "Hello" });
throw new Error("Test error");
}),
);
From the client the first call looks good, but the second one can't connect
$ curl http://localhost:8000/sse-error
data: Hello
$ curl http://localhost:8000/sse-error
curl: (7) Failed to connect to localhost port 8000 after 0 ms: Couldn't connect to server
The log on the server
<-- GET /sse-error
--> GET /sse-error 200 6ms
/project/src/index.ts:114
throw new Error("Test error");
^
Error: Test error
at <anonymous> (/project/src/index.ts:114:11)
at streamSSE (file:///project/node_modules/hono/dist/helper/streaming/sse.js:24:3)
at <anonymous> (/project/src/index.ts:112:3)
at dispatch (file:///project/node_modules/hono/dist/compose.js:29:23)
at file:///project/node_modules/hono/dist/compose.js:30:20
at logger2 (file:///project/node_modules/hono/dist/middleware/logger/index.js:35:11)
at dispatch (file:///project/node_modules/hono/dist/compose.js:29:23)
at file:///project/node_modules/hono/dist/compose.js:6:12
at file:///project/node_modules/hono/dist/hono-base.js:243:31
at Hono.dispatch (file:///project/node_modules/hono/dist/hono-base.js:253:7)
Node.js v20.9.0
What is the expected behavior?
I expect the server to keep on running even if I throw an exception. I will make sure that I catch all exceptions going forward, but still I think Hono could be more robust.
What do you see instead?
The server stops responding.
Additional information
No response
Unfortunately I could not reproduce it. May I ask what version of Node.js Adaptor you have as a possibility?
@watany-dev I've created a repro here: https://github.com/jonasb/hono-repro/tree/repro-2164
The server is still crashing for me. The thing about it stopping responding was that I was running the server with tsx --watch (npm run start-tsx-watch in the repro).
@jonasb Hi, sorry for the delay in replying due to my busy schedule. Thank you very much for reproducing it.
I was able to reproduce this in my environment and upon investigation found that the cb function of stream was not catching the throw.
This will certainly be a problem in the future and I would like to fix it.
I am considering putting these changes in with the fix, but I am still looking for a way to deliver the error to Hono's app.onError.
diff --git a/src/helper/streaming/stream.ts b/src/helper/streaming/stream.ts
index f82679d..ee1f251 100644
--- a/src/helper/streaming/stream.ts
+++ b/src/helper/streaming/stream.ts
@@ -4,6 +4,14 @@ import { StreamingApi } from '../../utils/stream'
export const stream = (c: Context, cb: (stream: StreamingApi) => Promise<void>): Response => {
const { readable, writable } = new TransformStream()
const stream = new StreamingApi(writable, readable)
- cb(stream).finally(() => stream.close())
+ ;(async () => {
+ try {
+ await cb(stream)
+ } catch (e) {
+ console.error(e) // want to pass this to `app.onError`
+ } finally {
+ stream.close()
+ }
+ })()
return c.newResponse(stream.responseReadable)
}
I've got the same problem. I noticed this when the Worker didn't catch an error that occurred while streaming. I rely on app.onError for all error handling and it didn't catch it or abort the stream.
The app.onError is a function to allow the response to be returned on the onError side when an error is thrown in the route handler, but since the ReadableStream is starting to respond when the stream is returned, the response cannot be overwritten, I can trigger onError, but I cannot respond to the handler's return of the onError argument. How would you handle this? @yusukebe @watany-dev
If an error occurs in the stream of a stream-response, it is difficult to return an error properly.
In the case of SSE, there is no specific specification for returning errors, so how about providing a method to handle errors such as onError?
app.get('/sse-error', (c) =>
streamSSE(
c,
async (stream) => {
stream.writeSSE({ data: JSON.stringify({ message: 'Hello' }) })
throw new Error('Test error')
},
async (e, stream) => {
await stream.writeSSE({
data: JSON.stringify({
error: e.message,
}),
})
}
)
)
Implementation:
diff --git a/src/helper/streaming/sse.ts b/src/helper/streaming/sse.ts
index 145d014..400afdb 100644
--- a/src/helper/streaming/sse.ts
+++ b/src/helper/streaming/sse.ts
@@ -42,10 +42,24 @@ const setSSEHeaders = (context: Context) => {
context.header('Connection', 'keep-alive')
}
-export const streamSSE = (c: Context, cb: (stream: SSEStreamingApi) => Promise<void>) => {
+export const streamSSE = (
+ c: Context,
+ cb: (stream: SSEStreamingApi) => Promise<void>,
+ onError?: (e: Error, stream: SSEStreamingApi) => Promise<void>
+) => {
const { readable, writable } = new TransformStream()
const stream = new SSEStreamingApi(writable, readable)
- cb(stream).finally(() => stream.close())
+ ;(async () => {
+ try {
+ await cb(stream)
+ } catch (e) {
+ if (e instanceof Error && onError) {
+ await onError(e, stream)
+ }
+ } finally {
+ stream.close()
+ }
+ })()
setSSEHeaders(c)
return c.newResponse(stream.responseReadable)
}
That’s a nice idea. Surely, we could do that. But I think we could just try-catch and wrap in the cb of the stream rather than doing that…
app.get('/sse-error', (c) =>
streamSSE(
c,
async (stream) => {
try {
stream.writeSSE({ data: JSON.stringify({ message: 'Hello' }) })
throw new Error('Test error')
} catch (e) {
await stream.writeSSE({
data: JSON.stringify({
error: e.message,
}),
})
}
},
)
)
Of course, that's fine, but I think we need to have a default error handler to make sure the server process doesn't hang up.
Indeed. Then I'll try to implement the policy of making the third argument of stream an error handler.