Race condition / response termination when using async and responding with a stream

[ikrestov]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the regression has not already been reported

Last working version

3.x.x

Stopped working in version

4.0.0-4.0.3

Node.js version

16-18

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

Arch 202206

💥 Regression Report

Undere some conditions fastify responds with an empty response, occasionally logging errors about premature termination or headers sent.

This was first encountered in compressed responses (fastify-compress) after upgrading to v4, when responses started coming back empty and premature termination/ socket closures where reported in logs.

Upon furthere investigation it seems that there is a regression after kReplySent was refactored, when using async handlers and sending a stream response via reply.send

The condition here

fastify/lib/wrapThenable.js

Line 21 in 00f4088

if (payload !== undefined || (reply.sent === false && reply.raw.headersSent === false && reply.request.raw.aborted === false)) {

runs before headers are sent, calls send(undefined) which responds before stream is written.

Potentially related to #4018, #3994,
Not related fastify/fastify-compress#215

Steps to Reproduce

Test in 282e8e0 or
Basic example:

const Fastify = require('./fastify.js')
const Readable = require('stream').Readable

const fastify = Fastify()

const payload = Buffer.from(`hello world `.repeat(1024))

// In wrapThenable.js:27
// reply.sent === false && reply.raw.headersSent === false
// so wrapThenable calls send(undefined)
fastify.get('/broken', async (request, reply) => {
  const stream = new Readable()
  stream._read = () => {}
  stream.push(payload)
  stream.push(null)
  await new Promise((resolve) => { setTimeout(resolve, 100).unref() })
  reply.header('Content-Type', 'text/plain').send(stream)
})

// return being crucial here
// reply.sent === true && reply.raw.headersSent === true
fastify.get('/ok', async (request, reply) => {
  const stream = new Readable()
  stream._read = () => {}
  stream.push(payload)
  stream.push(null)
  await new Promise((resolve) => { setTimeout(resolve, 100).unref() })
  return reply.header('Content-Type', 'text/plain').send(stream)
})

// reply.sent === true && reply.raw.headersSent === true
fastify.get('/ok2', async (request, reply) => {
  const stream = new Readable()
  stream._read = () => {}
  stream.push(payload)
  stream.push(null)
  return reply.header('Content-Type', 'text/plain').send(stream)
})

// reply.sent === true && reply.raw.headersSent === true
fastify.get('/ok3', async (request, reply) => {
  const stream = new Readable()
  stream._read = () => {}
  stream.push(payload)
  stream.push(null)
  reply.header('Content-Type', 'text/plain').send(stream)
})

fastify.listen({port: 3000})

Expected Behavior

Fastify responds with hello world... in broken handler.

reply.send(stream) is not raced by reply.send(undefined) in https://github.com/fastify/fastify/blob/main/lib/wrapThenable.js

[mcollina]

The return statement is crucial and part of the documented breaking changes. You have to do:

fastify.get('/ok', async (request, reply) => {
  const stream = new Readable()
  stream._read = () => {}
  stream.push(payload)
  stream.push(null)
  await new Promise((resolve) => { setTimeout(resolve, 100).unref() })
  reply.header('Content-Type', 'text/plain').send(stream)
  return reply // this is crucial
})

[ikrestov]

Apologies for the bug.

We didn't see it anywhere, .i.e. https://www.fastify.io/docs/latest/Guides/Migration-Guide-V4/ does not mention it.
I didn't see any deprecation in v3, so that is v4 breaking change right?
I see it was documented on routes page since v1 :(.

Should Typescript typings exclude undefined return type from handlers to help catch these issues? e.g.
T extends Promise<infer I> ? () => Promise<Exclude<I, undefined>> : () => Exclude<T, undefined>.

We kind of shot ourselvese in the foot with this, and only updated to return reply now.