http2: stream.pushStream callback signature changed in v8.11.2

[watson]

• Version: v8.11.2
• Platform: Darwin
• Subsystem: http2

The callback signature given to steam.pushStream() used to be function (stream, headers) {} in Node.js ^8.4.0. This was changed to function (err, stream, headers) {} in Node.js 9.

It seems that with the new release of v8.11.2 the new signature from Node.js 9 have made its way back to Node.js 8, which breaks all apps that's using it.

I'm still investigating the details and will follow up in this issue as I learn more.

Test program

const http2 = require('http2')

const server = http2.createServer()

server.on('stream', function (stream, headers) {
  stream.pushStream({':path': '/pushed'}, (stream, headers) => {
    stream.respond({
      'content-type': 'text/plain',
      ':status': 200
    })
    stream.end('some pushed data')
  })

  stream.respond({
    'content-type': 'text/plain',
    ':status': 200
  })
  stream.end('foo')
})

server.listen(() => {
  const client = http2.connect('http://localhost:' + server.address().port)

  client.on('stream', (stream, headers, flags) => {
    process.exit()
  })

  client.request({':path': '/'}).end()
})

[apapirovski]

Does this constitute an issue though? This is expected behaviour. http2 is experimental so changes are frequent and often breaking. We finally got around to porting a bunch of this stuff to v8.x hence the breakage.

[watson]

@apapirovski Good question. I'm not sure about our policy on this regarding experimental modules

[watson]

I would assume that it was a deliberate decision to not change the method signature until v9, so now that we did the backport in v8.11.2, it seems like it was a mistake to backport this specific change. But that's all based on assumptions.

[apapirovski]

I would assume that it was a deliberate decision to not change the method signature until v9

I don't think so. We had been stuck in back-porting a bunch of http2 stuff due to it depending on things not present in v8.x (I believe AliasedBuffer and native SetImmediate?).

[watson]

For reference, this is the commit containing the back-ported code that introduced it: fc40b7d#diff-696b2cc418addca5f3fe5020058f8b15R1951

[Flarna]

I'm quite sure that this is intended. See comments at #17406 (comment).

But I still see differences between 8.11.2 and 10.x in HTTP2, e.g. sequence of error and close events are exchanged for server streams; 10.x looks better to me as error comes first.

Is there anything we can track regarding HTTP2 backport? I found #18068 but it seems to be outdated.

[MylesBorins]

seems to me that this could be closed The breaking change was indeed intentional and the experimental contract of HTTP2 allows us to do so in a semver patch

@watson does that seem reasonable

[watson]

@MylesBorins That fine 👍I just wasn't sure if this was intentional or not.

I'm not sure if it's possible to do without too much overhead, but it would be nice if the release notes would highlight these breaking changes. But that's just a suggestion 😃