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: #29529
ronag · Dec 14, 2019 · Dec 14, 2019 · Dec 14, 2019 · Dec 14, 2019 · Jan 1, 2020
commit e09aa04ae5134c6ea3114715812cffd80c88332e
diff --git 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
@@ -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();
+      }));
     }));
   }));
-}));
+}