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: https://github.com/nodejs/node/pull/18936
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

7 files changed, 164 insertions, 14 deletions

diff --git a/doc/api/errors.md b/doc/api/errors.md
index 7d6e238f93..f5190dc52b 100644
--- 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
index 94f25377d0..13aa4d1e21 100644
--- 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
index 8ec72dd647..3d0b6cf9b3 100644
--- 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
index 607ae3fd2d..7137e527df 100644
--- 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
index 6818616227..24a6d2dc96 100644
--- 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
new file mode 100644
index 0000000000..eb782e2dea
--- /dev/null
+++ 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
new file mode 100644
index 0000000000..2b7ca3fc9a
--- /dev/null
+++ 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);
+}));