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. Backport-PR-URL: #31444 PR-URL: #30964 Refs: #29529 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
nodejs · Feb 6, 2020 · 92a9dac · 92a9dac
1 parent e2cd110
commit 92a9dac
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 40 deletions.
diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js
@@ -40,7 +40,8 @@ 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
   },
   hideStackFrames
 } = require('internal/errors');
@@ -439,6 +440,7 @@ class Http2ServerResponse extends Stream {
     this[kState] = {
       closed: false,
       ending: false,
+      destroyed: false,
       headRequest: false,
       sendDate: true,
       statusCode: HTTP_STATUS_OK,
@@ -649,23 +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].closed) {
-      const err = new ERR_HTTP2_INVALID_STREAM();
+    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;
+    }
+
+    if (err) {
       if (typeof cb === 'function')
         process.nextTick(cb, err);
-      else
-        throw err;
-      return;
+      this.destroy(err);
+      return false;
     }
 
     const stream = this[kStream];
     if (!stream.headersSent)
-      this.writeHead(this[kState].statusCode);
+      this.writeHead(state.statusCode);
     return stream.write(chunk, encoding, cb);
   }
 
@@ -712,8 +723,10 @@ class Http2ServerResponse extends Stream {
   }
 
   destroy(err) {
-    if (this[kState].closed)
+    if (this[kState].destroyed)
       return;
+
+    this[kState].destroyed = true;
     this[kStream].destroy(err);
   }
 

diff --git a/test/parallel/test-http2-compat-serverresponse-write.js b/test/parallel/test-http2-compat-serverresponse-write.js
@@ -3,50 +3,71 @@
 const {
   mustCall,
   mustNotCall,
-  expectsError,
   hasCrypto,
   skip
 } = require('../common');
 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();
+{
+  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()));
+    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.stream.close(0, mustCall(() => {
+        response.on('error', mustNotCall());
 
-      // response.write() without cb returns error
-      expectsError(
-        () => { response.write('muahaha'); },
-        {
-          type: Error,
-          code: 'ERR_HTTP2_INVALID_STREAM',
-          message: 'The stream has been destroyed'
-        }
-      );
+        // 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()));
+          // response.write() with cb returns falsy value
+          assert(!response.write('muahaha', mustCall()));
+
+          client.destroy();
+          server.close();
+        }));
+      }));
+    }));
+  }));
+}
+
+{
+  // 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();
+      }));
+    }));
 
-      client.destroy();
-      server.close();
+    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();
+      }));
     }));
   }));
-}));
+}