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

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()
})

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.

@apapirovski

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

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.

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?).

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

test(http2): fix broken tests on new Node v8.11.2 release

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.

test(http2): fix broken tests on new Node v8.11.2 release (#355)

@watson

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

@MylesBorins

@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 😃

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

apapirovski commented on May 16, 2018

watson commented on May 16, 2018

watson commented on May 16, 2018

apapirovski commented on May 16, 2018

watson commented on May 16, 2018

Flarna commented on May 16, 2018

MylesBorins commented on May 16, 2018

watson commented on May 16, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions