http2: remove regular-file-only restriction

Requiring `respondWithFile()` to only work with regular files is an artificial restriction on Node’s side and has become unnecessary. Offsets or lengths cannot be specified for those files, but that is an inherent property of other file types. PR-URL: #18936 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
nodejs · Mar 15, 2018 · 12b9ec0 · 12b9ec0
1 parent 1eb6b01
commit 12b9ec0
Show file tree

Hide file tree

Showing 7 changed files with 164 additions and 14 deletions.
diff --git a/doc/api/errors.md b/doc/api/errors.md
@@ -971,7 +971,14 @@ client.
 ### ERR_HTTP2_SEND_FILE
 
 An attempt was made to use the `Http2Stream.prototype.responseWithFile()` API to
-send something other than a regular file.
+send a directory.
+
+<a id="ERR_HTTP2_SEND_FILE_NOSEEK"></a>
+### ERR_HTTP2_SEND_FILE_NOSEEK
+
+An attempt was made to use the `Http2Stream.prototype.responseWithFile()` API to
+send something other than a regular file, but `offset` or `length` options were
+provided.
 
 <a id="ERR_HTTP2_SESSION_ERROR"></a>
 ### ERR_HTTP2_SESSION_ERROR

diff --git a/doc/api/http2.md b/doc/api/http2.md
@@ -1223,6 +1223,11 @@ if the `getTrailers` callback attempts to set such header fields.
 #### http2stream.respondWithFD(fd[, headers[, options]])
 <!-- YAML
 added: v8.4.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/18936
+    description: Any readable file descriptor, not necessarily for a
+                 regular file, is supported now.
 -->
 
 * `fd` {number} A readable file descriptor.
@@ -1313,6 +1318,11 @@ if the `getTrailers` callback attempts to set such header fields.
 #### http2stream.respondWithFile(path[, headers[, options]])
 <!-- YAML
 added: v8.4.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/18936
+    description: Any readable file, not necessarily a
+                 regular file, is supported now.
 -->
 
 * `path` {string|Buffer|URL}

diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -714,7 +714,9 @@ E('ERR_HTTP2_PING_LENGTH', 'HTTP2 ping payload must be 8 bytes', RangeError);
 E('ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED',
   'Cannot set HTTP/2 pseudo-headers', Error);
 E('ERR_HTTP2_PUSH_DISABLED', 'HTTP/2 client has disabled push streams', Error);
-E('ERR_HTTP2_SEND_FILE', 'Only regular files can be sent', Error);
+E('ERR_HTTP2_SEND_FILE', 'Directories cannot be sent', Error);
+E('ERR_HTTP2_SEND_FILE_NOSEEK',
+  'Offset or length can only be specified for regular files', Error);
 E('ERR_HTTP2_SESSION_ERROR', 'Session closed with error code %s', Error);
 E('ERR_HTTP2_SOCKET_BOUND',
   'The socket is already bound to an Http2Session', Error);

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
@@ -42,6 +42,7 @@ const {
     ERR_HTTP2_PING_LENGTH,
     ERR_HTTP2_PUSH_DISABLED,
     ERR_HTTP2_SEND_FILE,
+    ERR_HTTP2_SEND_FILE_NOSEEK,
     ERR_HTTP2_SESSION_ERROR,
     ERR_HTTP2_SETTINGS_CANCEL,
     ERR_HTTP2_SOCKET_BOUND,
@@ -2045,12 +2046,21 @@ function doSendFileFD(session, options, fd, headers, streamOptions, err, stat) {
   }
 
   if (!stat.isFile()) {
-    const err = new ERR_HTTP2_SEND_FILE();
-    if (onError)
-      onError(err);
-    else
-      this.destroy(err);
-    return;
+    const isDirectory = stat.isDirectory();
+    if (options.offset !== undefined || options.offset > 0 ||
+        options.length !== undefined || options.length >= 0 ||
+        isDirectory) {
+      const err = isDirectory ?
+        new ERR_HTTP2_SEND_FILE() : new ERR_HTTP2_SEND_FILE_NOSEEK();
+      if (onError)
+        onError(err);
+      else
+        this.destroy(err);
+      return;
+    }
+
+    options.offset = -1;
+    options.length = -1;
   }
 
   if (this.destroyed || this.closed) {
@@ -2075,12 +2085,14 @@ function doSendFileFD(session, options, fd, headers, streamOptions, err, stat) {
     return;
   }
 
-  statOptions.length =
-    statOptions.length < 0 ? stat.size - (+statOptions.offset) :
-      Math.min(stat.size - (+statOptions.offset),
-               statOptions.length);
+  if (stat.isFile()) {
+    statOptions.length =
+      statOptions.length < 0 ? stat.size - (+statOptions.offset) :
+        Math.min(stat.size - (+statOptions.offset),
+                 statOptions.length);
 
-  headers[HTTP2_HEADER_CONTENT_LENGTH] = statOptions.length;
+    headers[HTTP2_HEADER_CONTENT_LENGTH] = statOptions.length;
+  }
 
   processRespondWithFD(this, fd, headers,
                        options.offset | 0,

diff --git a/test/parallel/test-http2-respond-file-error-dir.js b/test/parallel/test-http2-respond-file-error-dir.js
@@ -15,7 +15,7 @@ server.on('stream', (stream) => {
       common.expectsError({
         code: 'ERR_HTTP2_SEND_FILE',
         type: Error,
-        message: 'Only regular files can be sent'
+        message: 'Directories cannot be sent'
       })(err);
 
       stream.respond({ ':status': 404 });

diff --git a/test/parallel/test-http2-respond-file-error-pipe-offset.js b/test/parallel/test-http2-respond-file-error-pipe-offset.js
@@ -0,0 +1,61 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+if (common.isWindows)
+  common.skip('no mkfifo on Windows');
+const child_process = require('child_process');
+const path = require('path');
+const fs = require('fs');
+const http2 = require('http2');
+const assert = require('assert');
+
+const tmpdir = require('../common/tmpdir');
+tmpdir.refresh();
+
+const pipeName = path.join(tmpdir.path, 'pipe');
+
+const mkfifo = child_process.spawnSync('mkfifo', [ pipeName ]);
+if (mkfifo.error && mkfifo.error.code === 'ENOENT') {
+  common.skip('missing mkfifo');
+}
+
+process.on('exit', () => fs.unlinkSync(pipeName));
+
+const server = http2.createServer();
+server.on('stream', (stream) => {
+  stream.respondWithFile(pipeName, {
+    'content-type': 'text/plain'
+  }, {
+    offset: 10,
+    onError(err) {
+      common.expectsError({
+        code: 'ERR_HTTP2_SEND_FILE_NOSEEK',
+        type: Error,
+        message: 'Offset or length can only be specified for regular files'
+      })(err);
+
+      stream.respond({ ':status': 404 });
+      stream.end();
+    },
+    statCheck: common.mustNotCall()
+  });
+});
+server.listen(0, () => {
+
+  const client = http2.connect(`http://localhost:${server.address().port}`);
+  const req = client.request();
+
+  req.on('response', common.mustCall((headers) => {
+    assert.strictEqual(headers[':status'], 404);
+  }));
+  req.on('data', common.mustNotCall());
+  req.on('end', common.mustCall(() => {
+    client.close();
+    server.close();
+  }));
+  req.end();
+});
+
+fs.writeFile(pipeName, 'Hello, world!\n', common.mustCall());
diff --git a/test/parallel/test-http2-respond-file-with-pipe.js b/test/parallel/test-http2-respond-file-with-pipe.js
@@ -0,0 +1,58 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+if (common.isWindows)
+  common.skip('no mkfifo on Windows');
+const child_process = require('child_process');
+const path = require('path');
+const fs = require('fs');
+const http2 = require('http2');
+const assert = require('assert');
+
+const tmpdir = require('../common/tmpdir');
+tmpdir.refresh();
+
+const pipeName = path.join(tmpdir.path, 'pipe');
+
+const mkfifo = child_process.spawnSync('mkfifo', [ pipeName ]);
+if (mkfifo.error && mkfifo.error.code === 'ENOENT') {
+  common.skip('missing mkfifo');
+}
+
+process.on('exit', () => fs.unlinkSync(pipeName));
+
+const server = http2.createServer();
+server.on('stream', (stream) => {
+  stream.respondWithFile(pipeName, {
+    'content-type': 'text/plain'
+  }, {
+    onError: common.mustNotCall(),
+    statCheck: common.mustCall()
+  });
+});
+
+server.listen(0, () => {
+  const client = http2.connect(`http://localhost:${server.address().port}`);
+  const req = client.request();
+
+  req.on('response', common.mustCall((headers) => {
+    assert.strictEqual(headers[':status'], 200);
+  }));
+  let body = '';
+  req.setEncoding('utf8');
+  req.on('data', (chunk) => body += chunk);
+  req.on('end', common.mustCall(() => {
+    assert.strictEqual(body, 'Hello, world!\n');
+    client.close();
+    server.close();
+  }));
+  req.end();
+});
+
+fs.open(pipeName, 'w', common.mustCall((err, fd) => {
+  assert.ifError(err);
+  fs.writeSync(fd, 'Hello, world!\n');
+  fs.closeSync(fd);
+}));