Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream.end doesn't send the last chunk on node 16 #38735

Open
anonrig opened this issue May 19, 2021 · 4 comments
Open

stream.end doesn't send the last chunk on node 16 #38735

anonrig opened this issue May 19, 2021 · 4 comments
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@anonrig
Copy link
Member

anonrig commented May 19, 2021

  • Version: 16.0.0
  • Platform: Mac, Linux
    For Mac: Darwin Yagizs-MacBook-Pro.local 20.3.0 Darwin Kernel Version 20.3.0: Thu Jan 21 00:06:51 PST 2021; root:xnu-7195.81.3~1/RELEASE_ARM64_T8101 arm64
    For Linux: Refer to Github CI tests from refactor: remove p-map malijs/mali#259
  • Subsystem: Big Sur, Github CI

What steps will reproduce the bug?

test.cb('res stream: trailer metadata set and also sent using res.end() should get 2nd', t => {
  t.plan(12)
  const APP_HOST = tu.getHost()

  function listStuff (ctx) {
    ctx.setStatus('foo', 'bar')
    ctx.res = hl(getArrayData())
      .map(d => {
        d.message = d.message.toUpperCase()
        return d
      })
      .on('end', () => {
        ctx.call.end({ bar: 'biz' })
      })
  }

  const app = new Mali(ARG_PROTO_PATH, 'ArgService')
  t.truthy(app)
  app.use({ listStuff })
  app.start(APP_HOST).then(server => {
    t.truthy(server)

    let metadata
    let status
    const client = new argproto.ArgService(APP_HOST, grpc.credentials.createInsecure())
    const call = client.listStuff({ message: 'Hello' })

    const resData = []
    call.on('data', d => {
      resData.push(d.message)
    })

    call.on('end', () => {
      setTimeout(() => {
        endTest()
      }, 500)
    })

    call.on('metadata', md => {
      metadata = md
    })

    call.on('status', s => {
      status = s
    })

    function endTest () {
      t.deepEqual(resData, ['1 FOO', '2 BAR', '3 ASD', '4 QWE', '5 RTY', '6 ZXC'])
      t.truthy(metadata)
      t.true(metadata instanceof grpc.Metadata)
      const header = metadata.getMap()
      t.is(header['content-type'], 'application/grpc+proto')
      t.truthy(header.date)
      t.truthy(status)
      t.true(typeof status.code === 'number')
      t.truthy(status.metadata)
      t.true(status.metadata instanceof grpc.Metadata)
      console.log('metadata', status.metadata)
      console.log('metadata > map', status.metadata.getMap())
      const trailer = status.metadata.getMap()
      t.deepEqual(trailer, {
        bar: 'biz'
      })
      app.close().then(() => t.end())
    }
  })
})

How often does it reproduce? Is there a required condition?

It has a consistent behavior.

What is the expected behavior?

It should work as expected as with Node 14

What do you see instead?

The following test succeeds on Node 14 but fails on 16.

Additional information

Please refer to my pull request on Mali malijs/mali#259 to see the Github CI output of Node 14 and 16.

@Ayase-252
Copy link
Member

Could you provide a minimal runnable repo?

@Ayase-252 Ayase-252 added stream Issues and PRs related to the stream subsystem. and removed stream Issues and PRs related to the stream subsystem. labels May 19, 2021
@anonrig
Copy link
Member Author

anonrig commented May 19, 2021

We were using a discontinued library called Highland and I replaced it with the following code which fixes the issue in Node 16. But now it is broken in Node 14. What am I missing in here, do you have any clue @Ayase-252 ?

function listStuff (ctx) {
    ctx.setStatus('foo', 'bar')
    const readable = new Stream.Readable({ objectMode: true, read () { return true } })
    const writable = new Stream.Writable({ objectMode: true })
    writable._write = (data, encoding, done) => done()
    readable.pipe(writable)
    readable.on('end', () => ctx.call.end({ bar: 'biz' }))
    ctx.res = readable
    getArrayData().map(i => readable.push({ message: i.message.toUpperCase() }))
    readable.push(null) // end readable stream by sending a null chunk
  }

@anonrig
Copy link
Member Author

anonrig commented May 19, 2021

It seems that on Node 16 readable.push(null) ends the stream, but on Node 14 it doesn't.

@Ayase-252 Ayase-252 added the stream Issues and PRs related to the stream subsystem. label May 20, 2021
@Ayase-252
Copy link
Member

@anonrig

I'm sorry, I don't have idea about it. I tried to recreate the issue by removing unrelated logic with stream, as following

const Stream = require('stream')

function getArrayData() {
  return ['1', '2', '3']
}

const readable = new Stream.Readable({ objectMode: true, read () { return true } })
const writable = new Stream.Writable({ objectMode: true })
writable._write = (data, encoding, done) => done()
readable.pipe(writable)
readable.on('end', () => console.log("end"));

getArrayData().map(i => readable.push({ message: i.toUpperCase() }))
readable.push(null) // end readable stream by sending a null chunk

Then running it with Node.js v14.17.0. It shows that readable is ended by readable.pull(null).

➜  node-38735-repo node -v
v14.17.0
➜  node-38735-repo node index.js
end

Did I miss some critical parts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

2 participants