-
-
Notifications
You must be signed in to change notification settings - Fork 336
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
fix: abort and headers already sent errors for the rest api #5722
Changes from all commits
68c5e2e
ba6b116
11134a2
476e7ea
e467e63
175fcb4
7ce0c92
513396f
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 |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import {ChainForkConfig} from "@lodestar/config"; | ||
import {ErrorAborted} from "@lodestar/utils"; | ||
import {Api, ReqTypes, routesData, getEventSerdes} from "../routes/events.js"; | ||
import {ServerRoutes} from "../../utils/server/index.js"; | ||
import {ServerApi} from "../../interfaces.js"; | ||
|
@@ -36,6 +37,9 @@ export function getRoutes(config: ChainForkConfig, api: ServerApi<Api>): ServerR | |
await new Promise<void>((resolve, reject) => { | ||
void api.eventstream(req.query.topics, controller.signal, (event) => { | ||
try { | ||
// If the request is already aborted, we don't need to send any more events. | ||
if (req.raw.destroyed) return; | ||
|
||
const data = eventSerdes.toJson(event); | ||
res.raw.write(serializeSSEEvent({event: event.type, data})); | ||
} catch (e) { | ||
|
@@ -49,7 +53,12 @@ export function getRoutes(config: ChainForkConfig, api: ServerApi<Api>): ServerR | |
// The client may disconnect and we need to clean the subscriptions. | ||
req.raw.once("close", () => resolve()); | ||
req.raw.once("end", () => resolve()); | ||
req.raw.once("error", (err) => reject(err)); | ||
req.raw.once("error", (err) => { | ||
if ((err as unknown as {code: string}).code === "ECONNRESET") { | ||
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. I reviewed our code and did some research on how It is invoking both the Another thing is that usually this would be handled by fastify which also just ignore errors with code But since the event API is not handled by fastify we need to do it ourselves and applying same pattern sounds good to me. We might even consider just resolving here instead of rejecting with abort error but I guess if this error is not logged due to it being ignored upstream this is also fine. 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. I feel we should abort here on 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. Yeah, sounds good to me 👍 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.
This might be related to 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. This is also simliar to issue reported here |
||
return reject(new ErrorAborted()); | ||
} | ||
return reject(err); | ||
}); | ||
}); | ||
|
||
// api.eventstream will never stop, so no need to ever call `res.raw.end();` | ||
|
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.
This check will try to avoid headers already sent scenario. Though that edge case is very tricky to reproduce. I tried a lot of ticks but was not able to produce it locally.
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.
Are you sure this handles all cases where this can happen, I was think maybe to check
res.raw.headersSent
before setting hearders but would also have to look into this more if that the correct condition to check forThere 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.
headers already sent error occurred, when you try to write to a response, which related request is completed. That completion can be because request was aborted, or server already finished writing response and closed the stream with
res.end()
.I can't see the later case happens in our code, as we write the response only from one place. So the only case left in my mind is the first one, so this condition will cover it.
If you can find a way to reproduce
headers already sent
scenario manually, please share then we can test.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.
Based on the stack trace in the issue, the error is not caused by us setting headers but something fastify does.
Looks like here res.writeHead, might be an issue on fastify or related to us not closing correcty.
But
res.raw.write
looks like a good candidate that could be causing this. If we are sure this does not cause unwanted side effects the change looks good.I just saw this once or twice on my end over a period of several months and it was reported by a user once. Seems to be highly timing specific, I tried to reproduce it consistently but no luck so far
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 don't see any side effects for this change.
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.
@nazarhussain I was going through logs from sim tests and it looks like the error still happens there.
See node-1-cl-lodestar.log from this sim tests run. This error seems to happen quite frequently in sim tests.
I would suggest we revert this change here as it does not solve the issue. It might also be a bug in fastify and not directly solvable on our end.
Edit: I created an issue to keep track of this