Streaming uploads cause truncated responses in node 16

[achingbrain]

Support plan

• is this issue currently blocking your project? (yes/no): yes
• is this issue affecting a production system? (yes/no): yes

Context

• node version: 16
• module version with issue: 20.1.4
• last module version without issue: Unsure
• environment (e.g. node, browser, native): node
• used with (e.g. hapi application, another framework, standalone, ...): hapi application
• any other relevant information:

What are you trying to achieve or the steps to reproduce?

Take a look at the following code sample. It starts a server, makes a request to the server, sends some data, and finally prints out the response.

It has two methods of reading the data from the client, one treating request.payload as an async iterator, one treating it as a readable stream.

const Hapi = require('@hapi/hapi')
const { PassThrough } = require('stream')
const http = require('http')

function receiveDataAsStream (request, stream) {
  request.payload.on('data', () => {
    console.info('receive data')
  })
  request.payload.on('end', () => {
    console.info('end data')

    // send result to client
    stream.end('hello world')
  })
}

async function receiveDataAsAsyncIterator (request, stream) {
  for await (const buf of request.payload) {
    console.info('receive data')
  }

  // send result to client
  stream.end('hello world')
}

async function main () {
  const server = Hapi.server({
    port: 0,
    host: 'localhost'
  })

  server.route({
    method: 'POST',
    path: '/',
    options: {
      payload: {
        output: 'stream'
      }
    },
    handler: (request, h) => {
      const stream = new PassThrough()

      // simulate async work
      process.nextTick(() => {
        // works on node 14 and node 16
        // receiveDataAsStream(request, stream)

        // works on node 14, breaks on node 16
        receiveDataAsAsyncIterator(request, stream).catch(err => console.error(err))
      })

      return h.response(stream)
    }
  })

  await server.start()

  // make a request
  const req = http.request({
    hostname: 'localhost',
    port: server.info.port,
    method: 'POST'
  }, (res) => {
    res.on('data', (chunk) => {
      console.info(chunk.toString())
    })
    res.on('end', () => {
      console.info('response ended')
      server.stop().catch(err => console.error(err))
    })
  })

  console.info('send data')
  req.end('{}')
}

main().catch(err => {
  console.error(err)
  process.exit(1)
})

On node 16 the 'hello world' output is not received by the client, on node 14 it is.

I think this is something to do with how the request payload is consumed as when you switch the receiveDataAsAsyncIterator function out for receiveDataAsStream it works as expected on node 14 and node 16.

What was the result you got?

Node 14:

$ node .
send data
receive data
hello world
response ended

Node 16:

$ node .
send data
receive data
response ended

The 'hello world' string is missing from the node 16 output.

What result did you expect?

Node 14:

$ node .
send data
receive data
hello world
response ended

Node 16:

$ node .
send data
receive data
hello world
response ended

The 'hello world' string should be included in the node 16 output.

[kanongil]

Thanks for the report! I have investigated the issue and can confirm that there is a bug.

This issue is due to the same change in node behavior that prompted #4225 and #4234. While that PR fixes the issue for the common cases, it does not fix it for stream responses.

What happens is that hapi will react to the incoming message being closed, and assume that this also applies to the outgoing response. This used to be true in node, but they changed it with v16 so it now closes right after the payload has been delivered.

I don't have time to implement a fix, but can inform that hello world is delivered if I comment out this line in internals.pipe():

hapi/lib/transmit.js

Line 260 in b1f6fd6

request.raw.req.on('close', close);

[devinivy]

I started digging into this and a question arose pretty quickly: do we believe that this test does not apply to node v16?

hapi/test/transmit.js

Line 1193 in b1f6fd6

it('stops processing the stream when the request closes', async () => {

[kanongil]

@devinivy Hmm, yeah – Given the new semantics, it does not apply as it it currently written.

I have tried to revise the test in a v16 compatible way in #4263.

[devinivy]

Resolved by #4264, published in v20.1.5 👍 Thanks for the report!