Skip to content
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: prevent eventstream errors on shutdown or client aborts #5784

Merged
merged 1 commit into from
Jul 21, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions packages/api/src/beacon/server/events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,13 @@ export function getRoutes(config: ChainForkConfig, api: ServerApi<Api>): ServerR
const controller = new AbortController();

try {
// Add injected headers from other pluggins. This is required for fastify-cors for example
// Prevent Fastify from sending the response, this is recommended before writing to the `.raw` stream
// and avoids "Cannot set headers after they are sent to the client" errors during shutdown or client aborts.
// See https://github.com/fastify/fastify/issues/3979, https://github.com/ChainSafe/lodestar/issues/5783
// eslint-disable-next-line @typescript-eslint/no-floating-promises
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hijack() is not async, see reply.js#L116

res.hijack();

// Add injected headers from other plugins. This is required for fastify-cors for example
// From: https://github.com/NodeFactoryIo/fastify-sse-v2/blob/b1686a979fbf655fb9936c0560294a0c094734d4/src/plugin.ts
Object.entries(res.getHeaders()).forEach(([key, value]) => {
if (value !== undefined) res.raw.setHeader(key, value);
Expand All @@ -37,9 +43,6 @@ 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;
Copy link
Member Author

@nflaig nflaig Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed as it did not resolve the issue (added in #5722)


const data = eventSerdes.toJson(event);
res.raw.write(serializeSSEEvent({event: event.type, data}));
} catch (e) {
Expand Down