From b575c6d3659d5e6807f4ba83e79f887bed759fe7 Mon Sep 17 00:00:00 2001 From: Benjamin Gruenbaum Date: Sat, 12 Feb 2022 20:46:37 +0200 Subject: [PATCH] module: prefer async/await in https imports --- lib/internal/modules/esm/fetch_module.js | 172 +++++++++-------------- test/es-module/test-http-imports.mjs | 11 ++ 2 files changed, 79 insertions(+), 104 deletions(-) diff --git a/lib/internal/modules/esm/fetch_module.js b/lib/internal/modules/esm/fetch_module.js index 2cbc6ea72d0934..f6c2fc8827aa73 100644 --- 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} 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 | CacheEntry>} */ @@ -47,11 +48,11 @@ 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, }); } @@ -59,11 +60,11 @@ 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,59 +99,47 @@ 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} @@ -158,58 +147,31 @@ function fetchWithRedirects(parsed) { 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 index a9a5204ffb7947..c09ee4c54bc2d3 100644 --- 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(); }