module: prefer async/await in https imports

PR-URL: nodejs#41950 Fixes: nodejs#41950 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
sxa · Mar 7, 2022 · 96510b3 · 96510b3
1 parent 8431fb9
commit 96510b3
Show file tree

Hide file tree

Showing 2 changed files with 79 additions and 104 deletions.
diff --git a/lib/internal/modules/esm/fetch_module.js b/lib/internal/modules/esm/fetch_module.js
@@ -1,26 +1,24 @@
 'use strict';
 const {
-  ArrayPrototypePush,
-  Promise,
+  ObjectPrototypeHasOwnProperty,
   PromisePrototypeThen,
-  PromiseResolve,
   SafeMap,
   StringPrototypeEndsWith,
   StringPrototypeSlice,
   StringPrototypeStartsWith,
 } = primordials;
 const {
-  Buffer: {
-    concat: BufferConcat
-  }
+  Buffer: { concat: BufferConcat },
 } = require('buffer');
 const {
   ERR_NETWORK_IMPORT_DISALLOWED,
   ERR_NETWORK_IMPORT_BAD_RESPONSE,
+  ERR_MODULE_NOT_FOUND,
 } = require('internal/errors').codes;
 const { URL } = require('internal/url');
 const net = require('net');
-
+const { once } = require('events');
+const { compose } = require('stream');
 /**
  * @typedef CacheEntry
  * @property {Promise<string> | string} resolvedHREF
@@ -32,6 +30,9 @@ const net = require('net');
  * Only for GET requests, other requests would need new Map
  * HTTP cache semantics keep diff caches
  *
+ * It caches either the promise or the cache entry since import.meta.url needs
+ * the value synchronously for the response location after all redirects.
+ *
  * Maps HREF to pending cache entry
  * @type {Map<string, Promise<CacheEntry> | CacheEntry>}
  */
@@ -47,23 +48,23 @@ let HTTPSAgent;
 function HTTPSGet(url, opts) {
   const https = require('https'); // [1]
   HTTPSAgent ??= new https.Agent({ // [2]
-    keepAlive: true
+    keepAlive: true,
   });
   return https.get(url, {
     agent: HTTPSAgent,
-    ...opts
+    ...opts,
   });
 }
 
 let HTTPAgent;
 function HTTPGet(url, opts) {
   const http = require('http'); // [1]
   HTTPAgent ??= new http.Agent({ // [2]
-    keepAlive: true
+    keepAlive: true,
   });
   return http.get(url, {
     agent: HTTPAgent,
-    ...opts
+    ...opts,
   });
 }
 
@@ -98,118 +99,79 @@ function fetchWithRedirects(parsed) {
     return existing;
   }
   const handler = parsed.protocol === 'http:' ? HTTPGet : HTTPSGet;
-  const result = new Promise((fulfill, reject) => {
+  const result = (async () => {
     const req = handler(parsed, {
-      headers: {
-        Accept: '*/*'
-      }
-    })
-    .on('error', reject)
-    .on('response', (res) => {
-      function dispose() {
-        req.destroy();
-        res.destroy();
-      }
-      if (res.statusCode >= 300 && res.statusCode <= 303) {
-        if (res.headers.location) {
-          dispose();
-          try {
-            const location = new URL(res.headers.location, parsed);
-            if (location.protocol !== 'http:' &&
-              location.protocol !== 'https:') {
-              reject(new ERR_NETWORK_IMPORT_DISALLOWED(
-                res.headers.location,
-                parsed.href,
-                'cannot redirect to non-network location'));
-              return;
-            }
-            return PromisePrototypeThen(
-              PromiseResolve(fetchWithRedirects(location)),
-              (entry) => {
-                cacheForGET.set(parsed.href, entry);
-                fulfill(entry);
-              });
-          } catch (e) {
-            dispose();
-            reject(e);
-          }
+      headers: { Accept: '*/*' },
+    });
+    // Note that `once` is used here to handle `error` and that it hits the
+    // `finally` on network error/timeout.
+    const { 0: res } = await once(req, 'response');
+    try {
+      const isRedirect = res.statusCode >= 300 && res.statusCode <= 303;
+      const hasLocation = ObjectPrototypeHasOwnProperty(res.headers, 'location');
+      if (isRedirect && hasLocation) {
+        const location = new URL(res.headers.location, parsed);
+        if (location.protocol !== 'http:' && location.protocol !== 'https:') {
+          throw new ERR_NETWORK_IMPORT_DISALLOWED(
+            res.headers.location,
+            parsed.href,
+            'cannot redirect to non-network location'
+          );
         }
+        const entry = await fetchWithRedirects(location);
+        cacheForGET.set(parsed.href, entry);
+        return entry;
+      }
+      if (res.statusCode === 404) {
+        const err = new ERR_MODULE_NOT_FOUND(parsed.href, null);
+        err.message = `Cannot find module '${parsed.href}', HTTP 404`;
+        throw err;
       }
       if (res.statusCode > 303 || res.statusCode < 200) {
-        dispose();
-        reject(
-          new ERR_NETWORK_IMPORT_BAD_RESPONSE(
-            parsed.href,
-            'HTTP response returned status code of ' + res.statusCode));
-        return;
+        throw new ERR_NETWORK_IMPORT_DISALLOWED(
+          res.headers.location,
+          parsed.href,
+          'cannot redirect to non-network location');
       }
       const { headers } = res;
       const contentType = headers['content-type'];
       if (!contentType) {
-        dispose();
-        reject(new ERR_NETWORK_IMPORT_BAD_RESPONSE(
+        throw new ERR_NETWORK_IMPORT_BAD_RESPONSE(
           parsed.href,
-          'the \'Content-Type\' header is required'));
-        return;
+          "the 'Content-Type' header is required"
+        );
       }
       /**
        * @type {CacheEntry}
        */
       const entry = {
         resolvedHREF: parsed.href,
         headers: {
-          'content-type': res.headers['content-type']
+          'content-type': res.headers['content-type'],
         },
-        body: new Promise((f, r) => {
-          const buffers = [];
-          let size = 0;
+        body: (async () => {
           let bodyStream = res;
-          let onError;
           if (res.headers['content-encoding'] === 'br') {
-            bodyStream = createBrotliDecompress();
-            onError = function onError(error) {
-              bodyStream.close();
-              dispose();
-              reject(error);
-              r(error);
-            };
-            res.on('error', onError);
-            res.pipe(bodyStream);
-          } else if (res.headers['content-encoding'] === 'gzip' ||
-            res.headers['content-encoding'] === 'deflate') {
-            bodyStream = createUnzip();
-            onError = function onError(error) {
-              bodyStream.close();
-              dispose();
-              reject(error);
-              r(error);
-            };
-            res.on('error', onError);
-            res.pipe(bodyStream);
-          } else {
-            onError = function onError(error) {
-              dispose();
-              reject(error);
-              r(error);
-            };
+            bodyStream = compose(res, createBrotliDecompress());
+          } else if (
+            res.headers['content-encoding'] === 'gzip' ||
+            res.headers['content-encoding'] === 'deflate'
+          ) {
+            bodyStream = compose(res, createUnzip());
           }
-          bodyStream.on('error', onError);
-          bodyStream.on('data', (d) => {
-            ArrayPrototypePush(buffers, d);
-            size += d.length;
-          });
-          bodyStream.on('end', () => {
-            const body = entry.body = /** @type {Buffer} */(
-              BufferConcat(buffers, size)
-            );
-            f(body);
-          });
-        }),
+          const buffers = await bodyStream.toArray();
+          const body = BufferConcat(buffers);
+          entry.body = body;
+          return body;
+        })(),
       };
       cacheForGET.set(parsed.href, entry);
-      fulfill(entry);
-    });
-  });
+      await entry.body;
+      return entry;
+    } finally {
+      req.destroy();
+    }
+  })();
   cacheForGET.set(parsed.href, result);
   return result;
 }
@@ -226,8 +188,10 @@ allowList.addRange('127.0.0.1', '127.255.255.255');
  */
 async function isLocalAddress(hostname) {
   try {
-    if (StringPrototypeStartsWith(hostname, '[') &&
-        StringPrototypeEndsWith(hostname, ']')) {
+    if (
+      StringPrototypeStartsWith(hostname, '[') &&
+      StringPrototypeEndsWith(hostname, ']')
+    ) {
       hostname = StringPrototypeSlice(hostname, 1, -1);
     }
     const addr = await dnsLookup(hostname, { verbatim: true });
@@ -275,5 +239,5 @@ function fetchModule(parsed, { parentURL }) {
 }
 
 module.exports = {
-  fetchModule: fetchModule
+  fetchModule: fetchModule,
 };
diff --git a/test/es-module/test-http-imports.mjs b/test/es-module/test-http-imports.mjs
@@ -64,6 +64,11 @@ for (const { protocol, createServer } of [
     const server = createServer(function(_req, res) {
       const url = new URL(_req.url, host);
       const redirect = url.searchParams.get('redirect');
+      if (url.pathname === '/not-found') {
+        res.writeHead(404);
+        res.end();
+        return;
+      }
       if (redirect) {
         const { status, location } = JSON.parse(redirect);
         res.writeHead(status, {
@@ -165,6 +170,12 @@ for (const { protocol, createServer } of [
       import(unsupportedMIME.href),
       { code: 'ERR_UNKNOWN_MODULE_FORMAT' }
     );
+    const notFound = new URL(url.href);
+    notFound.pathname = '/not-found';
+    await assert.rejects(
+      import(notFound.href),
+      { code: 'ERR_MODULE_NOT_FOUND' },
+    );
 
     server.close();
   }