Unparsed stream payload breaks under node 14

[achingbrain]

Support plan

• is this issue currently blocking your project? (yes/no): yes, blocking an upgrade to hapi@20.0.0
• is this issue affecting a production system? (yes/no): no, release version has not upgraded yet

Context

• node version: v14.8.0
• module version with issue: hapi@20.0.0
• last module version without issue: I tried hapi 17-20, the behaviour of all is the same under node 14
• 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?

Setting route options as:

options: {
  payload: {
    parse: false,
    output: 'stream'
  }
}

Works with node 12 but breaks with node 14.

A full example (using @hapi/hapi@20.0.0):

const Hapi = require('@hapi/hapi')
const server = Hapi.server()

server.route({
  method: 'POST',
  path: '/derp',
  options: {
    payload: {
      parse: false,
      output: 'stream'
    }
  },
  async handler (request, h) {
    console.info('start reading')

    try {
      for await (const chunk of request.payload) {
        console.info('got chunk', chunk.toString())
      }
    } catch (err) {
      console.error(err)
    }

    console.info('done reading')

    return 'ok'
  }
})

server.inject({
  method: 'POST',
  url: '/derp',
  payload: Buffer.from('hello world')
})
  .then(res => {
    console.info('got response', res.statusCode)
  })
  .catch(err => {
    console.error(err)
    process.exit(1)
  })

What was the result you got?

node@12:

$ node --version
v12.16.1
$ node test.js
start reading
got chunk hello world
done reading
got response 200
$ echo $?
0

node@14:

$ node --version
v14.8.0
$ node test.js
start reading
got chunk hello world
$ echo $?
0

Setting output to 'data' makes it work as expected in both node versions, but buffers the whole payload into memory so can't be used.

Keeping output as 'stream', setting parse to true and passing an appropriate content-type along with the message breaks in the same way as setting parse to false.

What result did you expect?

The same behaviour under node 12 and node 14 when output is 'stream' and parse is false.

[kanongil]

Interesting. This might be an inject(), aka hapijs/shot issue, though. Can you replicate it without it? If you can't, it will possibly be an issue with hapijs/subtext.

FYI, hapi does literally nothing with payload: { parse: false, output: 'stream' }. It just hands over the stream passed from node.

[achingbrain]

It does seem to work without .inject() on node 14:

const http = require('http')
const Hapi = require('@hapi/hapi')
const server = Hapi.server({
  port: 12345
})

server.route({
  method: 'POST',
  path: '/derp',
  options: {
    payload: {
      parse: true,
      output: 'stream'
    }
  },
  async handler (request, h) {
    console.info('start reading')

    for await (const chunk of request.payload) {
      console.info('got chunk', chunk.toString())
    }

    console.info('done reading')

    return 'ok'
  }
})

async function main () {
  await server.start()

  const req = http.request({
    hostname: '127.0.0.1',
    port: 12345,
    path: '/derp',
    method: 'POST'
  }, (res) => {
    console.info('got response', res.statusCode)
    res.resume()
    res.on('end', () => {
      server.stop()
        .catch(printErrorAndExit)
    })
  })
  req.write(Buffer.from('hello world'))
  req.end()
}

main()
  .catch(printErrorAndExit)

function printErrorAndExit (err) {
  console.error(err)
  process.exit(1)
}

$ node --version
v14.8.0
$ node test.js
start reading
got chunk hello world
done reading
got response 200
$ echo $?
0

[kanongil]

Thanks. I'm looking into fixing shot now.

[achingbrain]

It also works if I use events instead of for await..of in the handler:

const Hapi = require('@hapi/hapi')
const server = Hapi.server()

server.route({
  method: 'POST',
  path: '/derp',
  options: {
    payload: {
      parse: false,
      output: 'stream'
    }
  },
  async handler (request, h) {
    console.info('start reading')

    let done
    const promise = new Promise((resolve) => {
      done = resolve
    })

    request.payload.on('data', (chunk) => {
      console.info('got chunk', chunk.toString())
    })

    request.payload.on('end', () => {
      console.info('done reading')

      done('ok')
    })

    return promise
  }
})

server.inject({
  method: 'POST',
  url: '/derp',
  payload: Buffer.from('hello world')
})
  .then(res => {
    console.info('got response', res.statusCode)
  })
  .catch(err => {
    console.error(err)
    process.exit(1)
  })

$ node --version
v14.8.0
$ node test.js
start reading
got chunk hello world
done reading
got response 200

[achingbrain]

Commenting out the implementation of readable.destroy in shot fixes the problem.

The docs say:

Implementors should not override this method, but instead implement readable._destroy()

I guess this is why!

[kanongil]

Yeah, I have seen this issue with the EOS detection in the async iterator before.

Fix in hapijs/shot#129, awaiting someone to review & publish.

[cjihrig]

shot@5.0.2 has been published.

[achingbrain]

Great stuff, thanks for the speedy resolution!

[cjihrig]

shot@5.0.3 has been released, which reverted the previous change. We'll need to look at a different approach.

[achingbrain]

I've opened hapijs/shot#132 - it's a bit simpler than the original fix, notably it doesn't try to change the close event behaviour as that wasn't part of the problem I'm encountering.