Skip to content

Commit

Permalink
http2: do not emit our own close emit in Http2Stream
Browse files Browse the repository at this point in the history
Streams were recently updated to emit their own close event. The
Http2Stream was an exception because it included the close argument
with the close event. Refactor that to use the built in close.

PR-URL: nodejs#19451
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
jasnell committed Mar 22, 2018
1 parent ab75548 commit cef9097
Show file tree
Hide file tree
Showing 9 changed files with 18 additions and 21 deletions.
8 changes: 4 additions & 4 deletions doc/api/http2.md
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ Immediately terminates the `Http2Session` and the associated `net.Socket` or
`tls.TLSSocket`.

Once destroyed, the `Http2Session` will emit the `'close'` event. If `error`
is not undefined, an `'error'` event will be emitted immediately after the
is not undefined, an `'error'` event will be emitted immediately before the
`'close'` event.

If there are any remaining open `Http2Streams` associated with the
Expand Down Expand Up @@ -816,9 +816,9 @@ added: v8.4.0
The `'close'` event is emitted when the `Http2Stream` is destroyed. Once
this event is emitted, the `Http2Stream` instance is no longer usable.

The listener callback is passed a single argument specifying the HTTP/2 error
code specified when closing the stream. If the code is any value other than
`NGHTTP2_NO_ERROR` (`0`), an `'error'` event will also be emitted.
The HTTP/2 error code used when closing the stream can be retrieved using
the `http2stream.rstCode` property. If the code is any value other than
`NGHTTP2_NO_ERROR` (`0`), an `'error'` event will have also been emitted.

#### Event: 'error'
<!-- YAML
Expand Down
3 changes: 0 additions & 3 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -1503,7 +1503,6 @@ class Http2Stream extends Duplex {
constructor(session, options) {
options.allowHalfOpen = true;
options.decodeStrings = false;
options.emitClose = false;
super(options);
this[async_id_symbol] = -1;

Expand Down Expand Up @@ -1888,9 +1887,7 @@ class Http2Stream extends Duplex {
// will destroy if it has been closed and there are no other open or
// pending streams.
session[kMaybeDestroy]();
process.nextTick(emit, this, 'close', code);
callback(err);

}
// The Http2Stream can be destroyed if it has closed and if the readable
// side has received the final chunk.
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-http2-client-rststream-before-connect.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@ server.listen(0, common.mustCall(() => {
// Second call doesn't do anything.
req.close(closeCode + 1);

req.on('close', common.mustCall((code) => {
req.on('close', common.mustCall(() => {
assert.strictEqual(req.destroyed, true);
assert.strictEqual(code, closeCode);
assert.strictEqual(req.rstCode, closeCode);
server.close();
client.close();
}));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ server.listen(0, common.mustCall(() => {
message: 'test'
}));

req.on('close', common.mustCall((code) => {
req.on('close', common.mustCall(() => {
assert.strictEqual(req.rstCode, NGHTTP2_INTERNAL_ERROR);
assert.strictEqual(req.rstCode, NGHTTP2_INTERNAL_ERROR);
assert.strictEqual(code, NGHTTP2_INTERNAL_ERROR);
server.close();
client.close();
}));
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-http2-options-max-reserved-streams.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ server.on('stream', common.mustCall((stream) => {
pushedStream.respond();
pushedStream.on('aborted', common.mustCall());
pushedStream.on('error', common.mustNotCall());
pushedStream.on('close', common.mustCall((code) => {
assert.strictEqual(code, 8);
pushedStream.on('close', common.mustCall(() => {
assert.strictEqual(pushedStream.rstCode, 8);
countdown.dec();
}));
}));
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-http2-server-rst-before-respond.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ server.on('listening', common.mustCall(() => {
const client = h2.connect(`http://localhost:${server.address().port}`);
const req = client.request();
req.on('headers', common.mustNotCall());
req.on('close', common.mustCall((code) => {
assert.strictEqual(h2.constants.NGHTTP2_NO_ERROR, code);
req.on('close', common.mustCall(() => {
assert.strictEqual(h2.constants.NGHTTP2_NO_ERROR, req.rstCode);
server.close();
client.close();
}));
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-http2-server-rst-stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ server.listen(0, common.mustCall(() => {
':method': 'POST',
'rstcode': test[0]
});
req.on('close', common.mustCall((code) => {
assert.strictEqual(code, test[0]);
req.on('close', common.mustCall(() => {
assert.strictEqual(req.rstCode, test[0]);
countdown.dec();
}));
req.on('aborted', common.mustCall());
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-http2-too-large-headers.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ server.listen(0, common.mustCall(() => {
type: Error,
message: 'Stream closed with error code NGHTTP2_ENHANCE_YOUR_CALM'
}));
req.on('close', common.mustCall((code) => {
assert.strictEqual(code, NGHTTP2_ENHANCE_YOUR_CALM);
req.on('close', common.mustCall(() => {
assert.strictEqual(req.rstCode, NGHTTP2_ENHANCE_YOUR_CALM);
server.close();
client.close();
}));
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-http2-too-many-headers.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ server.listen(0, common.mustCall(() => {
type: Error,
message: 'Stream closed with error code NGHTTP2_ENHANCE_YOUR_CALM'
}));
req.on('close', common.mustCall((code) => {
assert.strictEqual(code, NGHTTP2_ENHANCE_YOUR_CALM);
req.on('close', common.mustCall(() => {
assert.strictEqual(req.rstCode, NGHTTP2_ENHANCE_YOUR_CALM);
server.close();
client.close();
}));
Expand Down

0 comments on commit cef9097

Please sign in to comment.