http2: make writeHead behave like HTTP/1.

PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
jasnell · Aug 13, 2017 · 2db82e0 · 2db82e0
1 parent 0d8bf4c
commit 2db82e0
Show file tree

Hide file tree

Showing 5 changed files with 29 additions and 34 deletions.
diff --git a/doc/api/http2.md b/doc/api/http2.md
@@ -2402,7 +2402,7 @@ buffer. Returns `false` if all or part of the data was queued in user memory.
 added: REPLACEME
 -->
 
-Does nothing. Added for parity with [HTTP/1]().
+Throws an error as the `'continue'` flow is not current implemented. Added for parity with [HTTP/1]().
 
 ### response.writeHead(statusCode[, statusMessage][, headers])
 <!-- YAML

diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js
@@ -386,11 +386,6 @@ class Http2ServerResponse extends Stream {
     headers[name] = String(value);
   }
 
-  flushHeaders() {
-    if (this[kStream].headersSent === false)
-      this[kBeginSend]();
-  }
-
   get statusMessage() {
     if (statusMessageWarned === false) {
       process.emitWarning(
@@ -403,6 +398,11 @@ class Http2ServerResponse extends Stream {
     return '';
   }
 
+  flushHeaders() {
+    if (this[kStream].headersSent === false)
+      this[kBeginSend]();
+  }
+
   writeHead(statusCode, statusMessage, headers) {
     if (typeof statusMessage === 'string' && statusMessageWarned === false) {
       process.emitWarning(
@@ -414,6 +414,12 @@ class Http2ServerResponse extends Stream {
     if (headers === undefined && typeof statusMessage === 'object') {
       headers = statusMessage;
     }
+
+    const stream = this[kStream];
+    if (stream.headersSent === true) {
+      throw new errors.Error('ERR_HTTP2_INFO_HEADERS_AFTER_RESPOND');
+    }
+
     if (headers) {
       const keys = Object.keys(headers);
       let key = '';
@@ -422,8 +428,9 @@ class Http2ServerResponse extends Stream {
         this.setHeader(key, headers[key]);
       }
     }
+
     this.statusCode = statusCode;
-    // TODO mcollina this should probably call sendInfo
+    this[kBeginSend]();
   }
 
   write(chunk, encoding, cb) {
@@ -487,26 +494,6 @@ class Http2ServerResponse extends Stream {
     stream.setTimeout(msecs, callback);
   }
 
-  sendContinue(headers) {
-    this.sendInfo(100, headers);
-  }
-
-  sendInfo(code, headers) {
-    const stream = this[kStream];
-    if (stream.headersSent === true) {
-      throw new errors.Error('ERR_HTTP2_INFO_HEADERS_AFTER_RESPOND');
-    }
-    if (headers && typeof headers !== 'object')
-      throw new errors.TypeError('ERR_HTTP2_HEADERS_OBJECT');
-    if (stream === undefined) return;
-    code |= 0;
-    if (code < 100 || code >= 200)
-      throw new errors.RangeError('ERR_HTTP2_INVALID_INFO_STATUS', code);
-
-    headers[constants.HTTP2_HEADER_STATUS] = code;
-    stream.respond(headers);
-  }
-
   createPushResponse(headers, callback) {
     const stream = this[kStream];
     if (stream === undefined) {
@@ -544,9 +531,9 @@ class Http2ServerResponse extends Stream {
     this.emit('finish');
   }
 
-  // added for parity with HTTP/1
   writeContinue() {
-    // TODO mcollina this should probably be sendContinue
+    // TODO mcollina check what is the continue flow
+    throw new Error('not implemented yet');
   }
 }
 

diff --git a/test/parallel/test-http2-compat-serverresponse-end.js b/test/parallel/test-http2-compat-serverresponse-end.js
@@ -46,7 +46,6 @@ const {
   const server = createServer(mustCall((request, response) => {
     strictEqual(response.finished, true);
     response.writeHead(HTTP_STATUS_OK, { foo: 'bar' });
-    response.flushHeaders();
     response.end(mustNotCall());
   }));
   server.listen(0, mustCall(() => {

diff --git a/test/parallel/test-http2-compat-serverresponse-flushheaders.js b/test/parallel/test-http2-compat-serverresponse-flushheaders.js
@@ -7,18 +7,24 @@ const h2 = require('http2');
 
 // Http2ServerResponse.flushHeaders
 
+let serverResponse;
+
 const server = h2.createServer();
 server.listen(0, common.mustCall(function() {
   const port = server.address().port;
   server.once('request', common.mustCall(function(request, response) {
     response.flushHeaders();
     response.flushHeaders(); // Idempotent
-    response.writeHead(400, { 'foo-bar': 'abc123' }); // Ignored
+    common.expectsError(() => {
+      response.writeHead(400, { 'foo-bar': 'abc123' });
+    }, {
+      code: 'ERR_HTTP2_INFO_HEADERS_AFTER_RESPOND'
+    });
 
     response.on('finish', common.mustCall(function() {
       server.close();
     }));
-    response.end();
+    serverResponse = response;
   }));
 
   const url = `http://localhost:${port}`;
@@ -33,6 +39,7 @@ server.listen(0, common.mustCall(function() {
     request.on('response', common.mustCall(function(headers, flags) {
       assert.strictEqual(headers['foo-bar'], undefined);
       assert.strictEqual(headers[':status'], 200);
+      serverResponse.end();
     }, 1));
     request.on('end', common.mustCall(function() {
       client.destroy();

diff --git a/test/parallel/test-http2-compat-serverresponse-writehead.js b/test/parallel/test-http2-compat-serverresponse-writehead.js
@@ -12,11 +12,13 @@ server.listen(0, common.mustCall(function() {
   const port = server.address().port;
   server.once('request', common.mustCall(function(request, response) {
     response.setHeader('foo-bar', 'def456');
-    response.writeHead(500);
     response.writeHead(418, { 'foo-bar': 'abc123' }); // Override
 
+    common.expectsError(() => { response.writeHead(300); }, {
+      code: 'ERR_HTTP2_INFO_HEADERS_AFTER_RESPOND'
+    });
+
     response.on('finish', common.mustCall(function() {
-      assert.doesNotThrow(() => { response.writeHead(300); });
       server.close();
     }));
     response.end();