Skip to content

Commit

Permalink
http2: do not crash on stream listener removal w/ destroyed session
Browse files Browse the repository at this point in the history
Do not crash when the session is no longer available.

Fixes: #29457

PR-URL: #29459
Backport-PR-URL: #29618
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  • Loading branch information
addaleax authored and BethGriggs committed Sep 25, 2019
1 parent 92a2f8b commit 559a8e3
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 4 deletions.
12 changes: 8 additions & 4 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -363,23 +363,27 @@ function sessionListenerRemoved(name) {
// Also keep track of listeners for the Http2Stream instances, as some events
// are emitted on those objects.
function streamListenerAdded(name) {
const session = this[kSession];
if (!session) return;
switch (name) {
case 'priority':
this[kSession][kNativeFields][kSessionPriorityListenerCount]++;
session[kNativeFields][kSessionPriorityListenerCount]++;
break;
case 'frameError':
this[kSession][kNativeFields][kSessionFrameErrorListenerCount]++;
session[kNativeFields][kSessionFrameErrorListenerCount]++;
break;
}
}

function streamListenerRemoved(name) {
const session = this[kSession];
if (!session) return;
switch (name) {
case 'priority':
this[kSession][kNativeFields][kSessionPriorityListenerCount]--;
session[kNativeFields][kSessionPriorityListenerCount]--;
break;
case 'frameError':
this[kSession][kNativeFields][kSessionFrameErrorListenerCount]--;
session[kNativeFields][kSessionFrameErrorListenerCount]--;
break;
}
}
Expand Down
31 changes: 31 additions & 0 deletions test/parallel/test-http2-stream-removelisteners-after-close.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const http2 = require('http2');

// Regression test for https://github.com/nodejs/node/issues/29457:
// HTTP/2 stream event listeners can be added and removed after the
// session has been destroyed.

const server = http2.createServer((req, res) => {
res.end('Hi!\n');
});

server.listen(0, common.mustCall(() => {
const client = http2.connect(`http://localhost:${server.address().port}`);
const headers = { ':path': '/' };
const req = client.request(headers);

req.on('close', common.mustCall(() => {
req.removeAllListeners();
req.on('priority', common.mustNotCall());
server.close();
}));

req.on('priority', common.mustNotCall());
req.on('error', common.mustCall());

client.destroy();
}));

0 comments on commit 559a8e3

Please sign in to comment.