Repository navigation
http2: stream.pushStream callback signature changed in v8.11.2 #20773
Description
- 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()
})
Activity
apapirovski commentedon May 16, 2018
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 commentedon May 16, 2018
@apapirovski Good question. I'm not sure about our policy on this regarding experimental modules
watson commentedon May 16, 2018
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 commentedon May 16, 2018
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 commentedon May 16, 2018
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
Flarna commentedon May 16, 2018
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)
MylesBorins commentedon May 16, 2018
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 commentedon May 16, 2018
@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 😃