docs: explain stream error handling by lundibundi · Pull Request #5746 · fastify/fastify

lundibundi · 2024-10-17T10:00:15Z

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

nechaido · 2024-10-17T10:02:20Z

+1 We have encountered this issue in production.

docs/Reference/Server.md

gurgunday · 2024-10-17T10:06:32Z

@Fdawgs I lost the link that showed the best practices for writing docs, do you happen to remember where they were, so I can link it?

Fdawgs · 2024-10-17T10:14:34Z

@Fdawgs I lost the link that showed the best practices for writing docs, do you happen to remember where they were, so I can link it?

https://fastify.dev/docs/latest/Guides/Style-Guide/

"You" is fine for any informal documentation in docs/Guides but not for anything in docs/Reference.

climba03003 · 2024-10-17T10:48:15Z

It is better to have some example showing this case. I may consider it as bug.

Fdawgs

Blocking until I get a chance to properly review, can spot a few grammatical errors.

lundibundi · 2024-10-17T13:03:27Z

Trying this out with fastify unit tests it looks like the part about if the error happens in the middle of the stream may not be correct - https://github.com/fastify/fastify/blob/main/lib/reply.js#L688-L693. I will update after further testing.

lundibundi · 2024-10-21T15:17:27Z

Removing the statement on stream/socket hangup as I was not able to reproduce this with Fastify tests. The logic in stream error handling looks sound so maybe something was wrong with the test setup (or jest or fastify.inject).
Added few tests that check current behavior.

docs/Reference/Server.md

test/request-error.test.js

climba03003

The whole test is based on the wrong concept of error handling.
When you explicitly set the Content-Type, we respect it even if it goes through the error handler. So, it does not serialize the object in this case.

You should either always explicit set the Content-Type in error handler,

import fastify from "fastify";
import { Readable } from "stream";

const app = fastify({ logger: true });
app.get('/', async function (request, reply) {
  const stream = new Readable({
    read() {
      this.push('hello')
    }
  })
  process.nextTick(() => stream.destroy(new Error('stream error')))
  await reply.type('application/text').send(stream)
})
app.setErrorHandler((err, req, reply) => {
  reply
    .code(400)
    .type('application/json')
    .send({ error: err.message })
})
app.listen({ port: 3000 })

or let fastify to guess the Content-Type in your route handler.

import fastify from "fastify";
import { Readable } from "stream";

const app = fastify({ logger: true });
app.get('/', async function (request, reply) {
  const stream = new Readable({
    read() {
      this.push('hello')
    }
  })
  process.nextTick(() => stream.destroy(new Error('stream error')))
  await reply.send(stream)
})
app.setErrorHandler((err, req, reply) => {
  reply.code(400)
  reply.send({ error: err.message })
})

lundibundi · 2024-10-23T11:51:41Z

@climba03003 thanks for the clarification, I'll update the documentation/test to mention explicit contentType instead of JSON.stringify.

mcollina

lgtm

lundibundi · 2024-11-15T12:37:05Z

@climba03003 @Fdawgs @gurgunday @mcollina I've updated the docs and test cases according to the discussion as well as pulled most recent main branch, please take a look.
Sorry for the delay.

mcollina

Tiny change

docs/Reference/Server.md

Co-authored-by: Matteo Collina <matteo.collina@gmail.com> Signed-off-by: James Sumners <321201+jsumners@users.noreply.github.com>

docs/Reference/Server.md

Co-authored-by: Frazer Smith <frazer.dev@icloud.com> Signed-off-by: Denys Otrishko <shishugi@gmail.com>

The base branch was changed.

metcoder95

Linting seems failing

Fdawgs

Documentation-wise this is now good.

Signed-off-by: Aras Abbasi <aras.abbasi@googlemail.com>

Uzlopak · 2025-09-26T09:08:45Z

@metcoder95
@gurgunday
@climba03003

I fixed the tests to run properly. NEeds another approve

gurgunday

lgtm

… error-parameter for setErrorHandler and errorHandler to unknown, but configurable via generic TError (fastify#6308) * types: loosen setErrorHandler and errorHandler types * set TError to unknown add generic to errorHandler * fix * remove obsolte type tst * add unit test, fix potential issue --------- Co-authored-by: Jean <110341611+jean-michelet@users.noreply.github.com> build(deps-dev): remove @fastify/pre-commit (fastify#6319) `@fastify/pre-commit` isn't configured or used in this repo as there is no `pre-commit` object in package.json. It won't run regardless as scripts are disabled. Signed-off-by: Frazer Smith <frazer.dev@icloud.com> Co-authored-by: Aras Abbasi <aras.abbasi@googlemail.com> ci: improve citgm workflows (fastify#6334) * ci: improve citgm workflows * fix remark docs: explain stream error handling (fastify#5746) * docs: explain stream error handling * Update docs/Reference/Server.md Co-authored-by: Matteo Collina <matteo.collina@gmail.com> Signed-off-by: James Sumners <321201+jsumners@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Frazer Smith <frazer.dev@icloud.com> Signed-off-by: Denys Otrishko <shishugi@gmail.com> * docs: explain stream error handling * fix test --------- Signed-off-by: James Sumners <321201+jsumners@users.noreply.github.com> Signed-off-by: Denys Otrishko <shishugi@gmail.com> Signed-off-by: Aras Abbasi <aras.abbasi@googlemail.com> Co-authored-by: James Sumners <321201+jsumners@users.noreply.github.com> Co-authored-by: Matteo Collina <matteo.collina@gmail.com> Co-authored-by: Frazer Smith <frazer.dev@icloud.com> Co-authored-by: Aras Abbasi <aras.abbasi@googlemail.com> fix: error throwing in reply (fastify#6299) * fix: instantiate readable stream error * Create reply-web-stream-locked.test.js Signed-off-by: Juan L. <juanf03@gmail.com> Signed-off-by: Juan Letamendia <juanf03@gmail.com> * fix test --------- Signed-off-by: Juan L. <juanf03@gmail.com> Signed-off-by: Juan Letamendia <juanf03@gmail.com> Co-authored-by: Aras Abbasi <aras.abbasi@googlemail.com> docs: create 2 frst chapters of the tutorial docs: nit docs: create a fastify server docs: create basic routes docs: improve chapter 4 refactor: improve readability and quote examples docs: decoration chapter public part docs: decoration chapter public part fix: typo fix: typo fix: markdown lint docs: document internals docs: validatio nand serialization docs: move section mapping reminder with routes declarations docs: add hooks chapter docs: remove internals involved sections docs: add error handling chapter fix: copilot suggestions

github-actions bot added the documentation Improvements or additions to documentation label Oct 17, 2024

gurgunday reviewed Oct 17, 2024

View reviewed changes

docs/Reference/Server.md Outdated Show resolved Hide resolved

Fdawgs self-requested a review October 17, 2024 10:15

gurgunday previously approved these changes Oct 17, 2024

View reviewed changes

Fdawgs requested changes Oct 17, 2024

View reviewed changes

metcoder95 reviewed Oct 23, 2024

View reviewed changes

docs/Reference/Server.md Outdated Show resolved Hide resolved

climba03003 reviewed Oct 23, 2024

View reviewed changes

test/request-error.test.js Outdated Show resolved Hide resolved

test/request-error.test.js Outdated Show resolved Hide resolved

climba03003 requested changes Oct 23, 2024

View reviewed changes

mcollina previously approved these changes Oct 25, 2024

View reviewed changes

docs: explain stream error handling

61aec9f

lundibundi force-pushed the docs-stream-error branch from 6db4dbe to 61aec9f Compare November 15, 2024 12:36

Merge branch 'main' into docs-stream-error

b3801ef

mcollina reviewed Jan 12, 2025

View reviewed changes

docs/Reference/Server.md Outdated Show resolved Hide resolved

Fdawgs requested review from climba03003 and metcoder95 January 16, 2025 01:49

jsumners and others added 2 commits January 16, 2025 11:55

Update docs/Reference/Server.md

55f7e75

Co-authored-by: Matteo Collina <matteo.collina@gmail.com> Signed-off-by: James Sumners <321201+jsumners@users.noreply.github.com>

Merge branch 'main' into docs-stream-error

d30bfa2

climba03003 previously approved these changes Jan 19, 2025

View reviewed changes

jsumners requested a review from Fdawgs January 22, 2025 11:42

Fdawgs reviewed Jan 22, 2025

View reviewed changes

docs/Reference/Server.md Outdated Show resolved Hide resolved

Fdawgs reviewed Jan 22, 2025

View reviewed changes

docs/Reference/Server.md Outdated Show resolved Hide resolved

Apply suggestions from code review

2d82974

Co-authored-by: Frazer Smith <frazer.dev@icloud.com> Signed-off-by: Denys Otrishko <shishugi@gmail.com>

lundibundi changed the base branch from main to 1.x January 28, 2025 12:18

lundibundi changed the base branch from 1.x to main January 28, 2025 12:18

Merge branch 'main' into docs-stream-error

d1f3e43

metcoder95 reviewed Feb 7, 2025

View reviewed changes

lundibundi and others added 2 commits February 11, 2025 14:47

docs: explain stream error handling

d139355

Merge branch 'main' into docs-stream-error

b3d6856

Fdawgs approved these changes Feb 18, 2025

View reviewed changes

climba03003 approved these changes Feb 19, 2025

View reviewed changes

Fdawgs requested review from gurgunday, mcollina and metcoder95 February 19, 2025 09:02

metcoder95 approved these changes Feb 19, 2025

View reviewed changes

gurgunday approved these changes Mar 12, 2025

View reviewed changes

Merge branch 'main' into docs-stream-error

49ca8be

Signed-off-by: Aras Abbasi <aras.abbasi@googlemail.com>

metcoder95 approved these changes Sep 26, 2025

View reviewed changes

fix test

1432b97

climba03003 approved these changes Sep 26, 2025

View reviewed changes

gurgunday approved these changes Sep 26, 2025

View reviewed changes

Uzlopak merged commit 34fea68 into fastify:main Sep 26, 2025
32 checks passed

Uh oh!

Conversation

lundibundi commented Oct 17, 2024

Checklist

Uh oh!

nechaido commented Oct 17, 2024

Uh oh!

Uh oh!

gurgunday commented Oct 17, 2024

Uh oh!

Fdawgs commented Oct 17, 2024

Uh oh!

climba03003 commented Oct 17, 2024

Uh oh!

Fdawgs left a comment

Choose a reason for hiding this comment

Uh oh!

lundibundi commented Oct 17, 2024

Uh oh!

lundibundi commented Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

climba03003 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lundibundi commented Oct 23, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

lundibundi commented Nov 15, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metcoder95 left a comment

Choose a reason for hiding this comment

Uh oh!

Fdawgs left a comment

Choose a reason for hiding this comment

Uh oh!

Uzlopak commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gurgunday left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

lundibundi commented Oct 21, 2024 •

edited

Loading

climba03003 left a comment •

edited

Loading

Uzlopak commented Sep 26, 2025 •

edited

Loading