docs: explain stream error handling#5746
Conversation
github-actions
Bot
added
the
documentation
|
+1 We have encountered this issue in production. |
|
@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? |
"You" is fine for any informal documentation in |
|
It is better to have some example showing this case. I may consider it as bug. |
Fdawgs
left a comment
Fdawgs
left a comment
There was a problem hiding this comment.
Blocking until I get a chance to properly review, can spot a few grammatical errors.
|
Trying this out with fastify unit tests it looks like the part about |
|
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). |
There was a problem hiding this comment.
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 })
})
|
@climba03003 thanks for the clarification, I'll update the documentation/test to mention explicit contentType instead of JSON.stringify. |
lundibundi
force-pushed
the
docs-stream-error
branch
from
6db4dbe to
61aec9f
Compare
|
@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. |
Co-authored-by: Matteo Collina <[email protected]> Signed-off-by: James Sumners <[email protected]>
Co-authored-by: Frazer Smith <[email protected]> Signed-off-by: Denys Otrishko <[email protected]>
lundibundi
dismissed stale reviews from climba03003, mcollina, and gurgunday
The base branch was changed.
Fdawgs
left a comment
Fdawgs
left a comment
There was a problem hiding this comment.
Documentation-wise this is now good.
Signed-off-by: Aras Abbasi <[email protected]>
|
@metcoder95 I fixed the tests to run properly. NEeds another approve |
… 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 <[email protected]> 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 <[email protected]> Co-authored-by: Aras Abbasi <[email protected]> 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 <[email protected]> Signed-off-by: James Sumners <[email protected]> * Apply suggestions from code review Co-authored-by: Frazer Smith <[email protected]> Signed-off-by: Denys Otrishko <[email protected]> * docs: explain stream error handling * fix test --------- Signed-off-by: James Sumners <[email protected]> Signed-off-by: Denys Otrishko <[email protected]> Signed-off-by: Aras Abbasi <[email protected]> Co-authored-by: James Sumners <[email protected]> Co-authored-by: Matteo Collina <[email protected]> Co-authored-by: Frazer Smith <[email protected]> Co-authored-by: Aras Abbasi <[email protected]> fix: error throwing in reply (fastify#6299) * fix: instantiate readable stream error * Create reply-web-stream-locked.test.js Signed-off-by: Juan L. <[email protected]> Signed-off-by: Juan Letamendia <[email protected]> * fix test --------- Signed-off-by: Juan L. <[email protected]> Signed-off-by: Juan Letamendia <[email protected]> Co-authored-by: Aras Abbasi <[email protected]> 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
… 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 <[email protected]> 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 <[email protected]> Co-authored-by: Aras Abbasi <[email protected]> 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 <[email protected]> Signed-off-by: James Sumners <[email protected]> * Apply suggestions from code review Co-authored-by: Frazer Smith <[email protected]> Signed-off-by: Denys Otrishko <[email protected]> * docs: explain stream error handling * fix test --------- Signed-off-by: James Sumners <[email protected]> Signed-off-by: Denys Otrishko <[email protected]> Signed-off-by: Aras Abbasi <[email protected]> Co-authored-by: James Sumners <[email protected]> Co-authored-by: Matteo Collina <[email protected]> Co-authored-by: Frazer Smith <[email protected]> Co-authored-by: Aras Abbasi <[email protected]> fix: error throwing in reply (fastify#6299) * fix: instantiate readable stream error * Create reply-web-stream-locked.test.js Signed-off-by: Juan L. <[email protected]> Signed-off-by: Juan Letamendia <[email protected]> * fix test --------- Signed-off-by: Juan L. <[email protected]> Signed-off-by: Juan Letamendia <[email protected]> Co-authored-by: Aras Abbasi <[email protected]> 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
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
9 participants
Checklist
npm run testandnpm run benchmarkand the Code of conduct