fix(node): prevent crash on stream error #9533
Conversation
🦋 Changeset detectedLatest commit: 9d64ede The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
pkg: integration
| reader.cancel(); | ||
| // Cancelling the reader may reject not just because of | ||
| // an error in the ReadableStream's cancel callback, but | ||
| // also because of an error anywhere in the stream. | ||
| reader.cancel().catch(err => { | ||
| logger.error(`There was an uncaught error in the middle of the stream while rendering ${res.req.url}.`); | ||
| console.error(err); | ||
| }); |
There was a problem hiding this comment.
the fix is here, the rest is clerical
github-actions
bot
added
the
pr: docs
| } catch (err: any) { | ||
| console.error(err?.stack || err?.message || String(err)); | ||
| res.write('Internal server error'); |
There was a problem hiding this comment.
This could now potentially swallow errors without logging. Should keep the logging here as before?
There was a problem hiding this comment.
That would end up logging twice, because stream errors are also thrown on cancelling the reader.
There was a problem hiding this comment.
I guess I was worried that res.on('close' isn't always guaranteed to be called, which means reader.cancel() may not be called. So it feels safer to log it in this catch block instead. Maybe we can do reader.cancel().catch(() => {}) instead since we know the error should already be logged here?
There was a problem hiding this comment.
Wouldn't res.end() below this catch block guarantee that?
There was a problem hiding this comment.
True. In that case, maybe we should leave a comment here instead about why we're swallowing the error (also remove the (err: any) part)
Changes
Testing
node packages/integrations/node/test/fixtures/errors/dist/server/entry.mjscurl -i localhost:4321/in-streamDocs
Does not affect usage