From e09aa04ae5134c6ea3114715812cffd80c88332e Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 14 Dec 2019 19:09:23 +0100 Subject: [PATCH 1/4] http2: make HTTP2ServerResponse more streams compliant HTTP2ServerResponse.write would behave differently than both http1 and streams. This PR makes it more compliant with stream.Writable behaviour. Refs: https://github.com/nodejs/node/issues/29529 --- lib/internal/http2/compat.js | 39 +++++-- .../test-http2-compat-serverresponse-write.js | 108 +++++++++++++----- 2 files changed, 108 insertions(+), 39 deletions(-) diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 94282cc4bb572a..9f030f76d414f9 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -40,7 +40,9 @@ const { ERR_HTTP2_STATUS_INVALID, ERR_INVALID_ARG_VALUE, ERR_INVALID_CALLBACK, - ERR_INVALID_HTTP_TOKEN + ERR_INVALID_HTTP_TOKEN, + ERR_STREAM_WRITE_AFTER_END, + ERR_STREAM_DESTROYED }, hideStackFrames } = require('internal/errors'); @@ -61,6 +63,8 @@ const kAborted = Symbol('aborted'); let statusMessageWarned = false; let statusConnectionHeaderWarned = false; +function nop() {} + // Defines and implements an API compatibility layer on top of the core // HTTP/2 implementation, intended to provide an interface that is as // close as possible to the current require('http') API @@ -439,6 +443,7 @@ class Http2ServerResponse extends Stream { this[kState] = { closed: false, ending: false, + destroyed: false, headRequest: false, sendDate: true, statusCode: HTTP_STATUS_OK, @@ -649,17 +654,28 @@ class Http2ServerResponse extends Stream { } write(chunk, encoding, cb) { + const state = this[kState]; + if (typeof encoding === 'function') { cb = encoding; encoding = 'utf8'; } - if (this[kState].closed) { - const err = new ERR_HTTP2_INVALID_STREAM(); - if (typeof cb === 'function') - process.nextTick(cb, err); - else - throw err; + if (typeof cb !== 'function') + cb = nop; + + let err; + if (state.ending) { + err = new ERR_STREAM_WRITE_AFTER_END(); + } else if (state.destroyed) { + err = new ERR_STREAM_DESTROYED('write'); + } else if (state.closed) { + err = new ERR_HTTP2_INVALID_STREAM(); + } + + if (err) { + process.nextTick(cb, err); + this.destroy(err); return; } @@ -712,9 +728,14 @@ class Http2ServerResponse extends Stream { } destroy(err) { - if (this[kState].closed) + const stream = this[kStream]; + const state = this[kState]; + + if (state.destroyed) return; - this[kStream].destroy(err); + + state.destroyed = true; + stream.destroy(err); } setTimeout(msecs, callback) { diff --git a/test/parallel/test-http2-compat-serverresponse-write.js b/test/parallel/test-http2-compat-serverresponse-write.js index 6bec7c6c5eafe1..a4fd210b1b90ca 100644 --- a/test/parallel/test-http2-compat-serverresponse-write.js +++ b/test/parallel/test-http2-compat-serverresponse-write.js @@ -10,42 +10,90 @@ if (!hasCrypto) skip('missing crypto'); const { createServer, connect } = require('http2'); const assert = require('assert'); +{ + const server = createServer(); + server.listen(0, mustCall(() => { + const port = server.address().port; + const url = `http://localhost:${port}`; + const client = connect(url, mustCall(() => { + const request = client.request(); + request.resume(); + request.on('end', mustCall()); + request.on('close', mustCall(() => { + client.close(); + })); + })); + + server.once('request', mustCall((request, response) => { + // response.write() returns true + assert(response.write('muahaha', 'utf8', mustCall())); + + response.stream.close(0, mustCall(() => { + response.on('error', mustNotCall()); + + // response.write() without cb returns error + response.write('muahaha', mustCall((err) => { + assert.strictEqual(err.code, 'ERR_HTTP2_INVALID_STREAM'); + + // response.write() with cb returns falsy value + assert(!response.write('muahaha', mustCall())); -const server = createServer(); -server.listen(0, mustCall(() => { - const port = server.address().port; - const url = `http://localhost:${port}`; - const client = connect(url, mustCall(() => { - const request = client.request(); - request.resume(); - request.on('end', mustCall()); - request.on('close', mustCall(() => { - client.close(); + client.destroy(); + server.close(); + })); + })); })); })); +} - server.once('request', mustCall((request, response) => { - // response.write() returns true - assert(response.write('muahaha', 'utf8', mustCall())); - - response.stream.close(0, mustCall(() => { - response.on('error', mustNotCall()); +{ + // Http2ServerResponse.write ERR_STREAM_WRITE_AFTER_END + const server = createServer(); + server.listen(0, mustCall(() => { + const port = server.address().port; + const url = `http://localhost:${port}`; + const client = connect(url, mustCall(() => { + const request = client.request(); + request.resume(); + request.on('end', mustCall()); + request.on('close', mustCall(() => { + client.close(); + })); + })); - // response.write() without cb returns error - assert.throws( - () => { response.write('muahaha'); }, - { - name: 'Error', - code: 'ERR_HTTP2_INVALID_STREAM', - message: 'The stream has been destroyed' - } - ); + server.once('request', mustCall((request, response) => { + response.end(); + response.write('asd', mustCall((err) => { + assert.strictEqual(err.code, 'ERR_STREAM_WRITE_AFTER_END'); + client.destroy(); + server.close(); + })); + })); + })); +} - // response.write() with cb returns falsy value - assert(!response.write('muahaha', mustCall())); +{ + // Http2ServerResponse.destroy ERR_STREAM_DESTROYED + const server = createServer(); + server.listen(0, mustCall(() => { + const port = server.address().port; + const url = `http://localhost:${port}`; + const client = connect(url, mustCall(() => { + const request = client.request(); + request.resume(); + request.on('end', mustCall()); + request.on('close', mustCall(() => { + client.close(); + })); + })); - client.destroy(); - server.close(); + server.once('request', mustCall((request, response) => { + response.destroy(); + response.write('asd', mustCall((err) => { + assert.strictEqual(err.code, 'ERR_STREAM_DESTROYED'); + client.destroy(); + server.close(); + })); })); })); -})); +} From 4684a1ee267cd833769b4b37092c4948e7e53bbb Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 14 Dec 2019 20:42:01 +0100 Subject: [PATCH 2/4] fixup: align with http1 instead of streams --- lib/internal/http2/compat.js | 44 +++++++------------ .../test-http2-compat-serverresponse-write.js | 2 +- 2 files changed, 18 insertions(+), 28 deletions(-) diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 9f030f76d414f9..c122645059273f 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -41,8 +41,7 @@ const { ERR_INVALID_ARG_VALUE, ERR_INVALID_CALLBACK, ERR_INVALID_HTTP_TOKEN, - ERR_STREAM_WRITE_AFTER_END, - ERR_STREAM_DESTROYED + ERR_STREAM_WRITE_AFTER_END }, hideStackFrames } = require('internal/errors'); @@ -63,8 +62,6 @@ const kAborted = Symbol('aborted'); let statusMessageWarned = false; let statusConnectionHeaderWarned = false; -function nop() {} - // Defines and implements an API compatibility layer on top of the core // HTTP/2 implementation, intended to provide an interface that is as // close as possible to the current require('http') API @@ -654,29 +651,25 @@ class Http2ServerResponse extends Stream { } write(chunk, encoding, cb) { - const state = this[kState]; - if (typeof encoding === 'function') { cb = encoding; encoding = 'utf8'; } - if (typeof cb !== 'function') - cb = nop; - - let err; - if (state.ending) { - err = new ERR_STREAM_WRITE_AFTER_END(); - } else if (state.destroyed) { - err = new ERR_STREAM_DESTROYED('write'); - } else if (state.closed) { - err = new ERR_HTTP2_INVALID_STREAM(); - } - - if (err) { - process.nextTick(cb, err); + if (this[kState].ending) { + const err = new ERR_STREAM_WRITE_AFTER_END(); + if (typeof cb === 'function') + process.nextTick(cb, err); this.destroy(err); - return; + return false; + } else if (this[kState].closed) { + const err = new ERR_HTTP2_INVALID_STREAM(); + if (typeof cb === 'function') + process.nextTick(cb, err); + this.destroy(err); + return false; + } else if (this[kState].destroyed) { + return false; } const stream = this[kStream]; @@ -728,14 +721,11 @@ class Http2ServerResponse extends Stream { } destroy(err) { - const stream = this[kStream]; - const state = this[kState]; - - if (state.destroyed) + if (this[kState].destroyed) return; - state.destroyed = true; - stream.destroy(err); + this[kState].destroyed = true; + this[kStream].destroy(err); } setTimeout(msecs, callback) { diff --git a/test/parallel/test-http2-compat-serverresponse-write.js b/test/parallel/test-http2-compat-serverresponse-write.js index a4fd210b1b90ca..50ab0d1fd8b03f 100644 --- a/test/parallel/test-http2-compat-serverresponse-write.js +++ b/test/parallel/test-http2-compat-serverresponse-write.js @@ -90,7 +90,7 @@ const assert = require('assert'); server.once('request', mustCall((request, response) => { response.destroy(); response.write('asd', mustCall((err) => { - assert.strictEqual(err.code, 'ERR_STREAM_DESTROYED'); + assert.strictEqual(err.code, null); client.destroy(); server.close(); })); From 2f0d4b109250179fa74f01a5df6f8a525cf1c23b Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 14 Dec 2019 20:55:21 +0100 Subject: [PATCH 3/4] fixup: cleaner version --- lib/internal/http2/compat.js | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index c122645059273f..8ef5f49a3dbd77 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -651,30 +651,32 @@ class Http2ServerResponse extends Stream { } write(chunk, encoding, cb) { + const state = this[kState]; + if (typeof encoding === 'function') { cb = encoding; encoding = 'utf8'; } - if (this[kState].ending) { - const err = new ERR_STREAM_WRITE_AFTER_END(); - if (typeof cb === 'function') - process.nextTick(cb, err); - this.destroy(err); + let err; + if (state.ending) { + err = new ERR_STREAM_WRITE_AFTER_END(); + } else if (state.closed) { + err = new ERR_HTTP2_INVALID_STREAM(); + } else if (state.destroyed) { return false; - } else if (this[kState].closed) { - const err = new ERR_HTTP2_INVALID_STREAM(); + } + + if (err) { if (typeof cb === 'function') process.nextTick(cb, err); this.destroy(err); return false; - } else if (this[kState].destroyed) { - return false; } const stream = this[kStream]; if (!stream.headersSent) - this.writeHead(this[kState].statusCode); + this.writeHead(state.statusCode); return stream.write(chunk, encoding, cb); } From 9da63beba20acc777d814f5aa6a3eb19175e55d9 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 15 Dec 2019 00:04:53 +0100 Subject: [PATCH 4/4] fixup: removed destroyed test --- .../test-http2-compat-serverresponse-write.js | 26 ------------------- 1 file changed, 26 deletions(-) diff --git a/test/parallel/test-http2-compat-serverresponse-write.js b/test/parallel/test-http2-compat-serverresponse-write.js index 50ab0d1fd8b03f..1d7159d46e72bd 100644 --- a/test/parallel/test-http2-compat-serverresponse-write.js +++ b/test/parallel/test-http2-compat-serverresponse-write.js @@ -71,29 +71,3 @@ const assert = require('assert'); })); })); } - -{ - // Http2ServerResponse.destroy ERR_STREAM_DESTROYED - const server = createServer(); - server.listen(0, mustCall(() => { - const port = server.address().port; - const url = `http://localhost:${port}`; - const client = connect(url, mustCall(() => { - const request = client.request(); - request.resume(); - request.on('end', mustCall()); - request.on('close', mustCall(() => { - client.close(); - })); - })); - - server.once('request', mustCall((request, response) => { - response.destroy(); - response.write('asd', mustCall((err) => { - assert.strictEqual(err.code, null); - client.destroy(); - server.close(); - })); - })); - })); -}