http2: fix end without read

apapirovski · MylesBorins · commit b2fb1d70bb2a · 2018-05-22T14:13:28.000-04:00
Adjust http2 behaviour to allow ending a stream even after some data comes in (when the user has no intention of reading that data). Also correctly end a stream when trailers are present. PR-URL: #20621 Fixes: #20060 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js
@@ -260,8 +260,6 @@ class Http2ServerRequest extends Readable {
     stream[kRequest] = this;
 
     // Pause the stream..
-    stream.pause();
-    stream.on('data', onStreamData);
     stream.on('trailers', onStreamTrailers);
     stream.on('end', onStreamEnd);
     stream.on('error', onStreamError);
@@ -328,8 +326,12 @@ class Http2ServerRequest extends Readable {
   _read(nread) {
     const state = this[kState];
     if (!state.closed) {
-      state.didRead = true;
-      process.nextTick(resumeStream, this[kStream]);
+      if (!state.didRead) {
+        state.didRead = true;
+        this[kStream].on('data', onStreamData);
+      } else {
+        process.nextTick(resumeStream, this[kStream]);
+      }
     } else {
       this.emit('error', new ERR_HTTP2_INVALID_STREAM());
     }
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
@@ -349,11 +349,11 @@ function onStreamClose(code) {
     // Push a null so the stream can end whenever the client consumes
     // it completely.
     stream.push(null);
-
-    // Same as net.
-    if (stream.readableLength === 0) {
-      stream.read(0);
-    }
+    // If the client hasn't tried to consume the stream and there is no
+    // resume scheduled (which would indicate they would consume in the future),
+    // then just dump the incoming data so that the stream can be destroyed.
+    if (!stream[kState].didRead && !stream._readableState.resumeScheduled)
+      stream.resume();
   }
 }
 
@@ -1795,6 +1795,8 @@ class Http2Stream extends Duplex {
     const ret = this[kHandle].trailers(headersList);
     if (ret < 0)
       this.destroy(new NghttpError(ret));
+    else
+      this[kMaybeDestroy]();
   }
 
   get closed() {
diff --git a/test/parallel/test-http2-client-upload-reject.js b/test/parallel/test-http2-client-upload-reject.js
@@ -20,12 +20,15 @@ fs.readFile(loc, common.mustCall((err, data) => {
   const server = http2.createServer();
 
   server.on('stream', common.mustCall((stream) => {
-    stream.on('close', common.mustCall(() => {
-      assert.strictEqual(stream.rstCode, 0);
-    }));
-
-    stream.respond({ ':status': 400 });
-    stream.end();
+    // Wait for some data to come through.
+    setImmediate(() => {
+      stream.on('close', common.mustCall(() => {
+        assert.strictEqual(stream.rstCode, 0);
+      }));
+
+      stream.respond({ ':status': 400 });
+      stream.end();
+    });
   }));
 
   server.listen(0, common.mustCall(() => {
diff --git a/test/parallel/test-http2-compat-client-upload-reject.js b/test/parallel/test-http2-compat-client-upload-reject.js
@@ -0,0 +1,44 @@
+'use strict';
+
+// Verifies that uploading data from a client works
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const http2 = require('http2');
+const fs = require('fs');
+const fixtures = require('../common/fixtures');
+
+const loc = fixtures.path('person-large.jpg');
+
+assert(fs.existsSync(loc));
+
+fs.readFile(loc, common.mustCall((err, data) => {
+  assert.ifError(err);
+
+  const server = http2.createServer(common.mustCall((req, res) => {
+    setImmediate(() => {
+      res.writeHead(400);
+      res.end();
+    });
+  }));
+
+  server.listen(0, common.mustCall(() => {
+    const client = http2.connect(`http://localhost:${server.address().port}`);
+
+    const req = client.request({ ':method': 'POST' });
+    req.on('response', common.mustCall((headers) => {
+      assert.strictEqual(headers[':status'], 400);
+    }));
+
+    req.resume();
+    req.on('end', common.mustCall(() => {
+      server.close();
+      client.close();
+    }));
+
+    const str = fs.createReadStream(loc);
+    str.pipe(req);
+  }));
+}));
