Skip to content

[file_packager.py] Fix errback call in fetchRemotePackage #24871

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 12 commits into from
Closed

[file_packager.py] Fix errback call in fetchRemotePackage #24871

Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a423463
[file_packager.py] Fix `errback` in `fetchRemotePackage`
lkwinta Aug 6, 2025
36a127e
Merge branch 'main' into file_packager_errback
lkwinta Aug 6, 2025
72abcbd
Fix all Promise.reject calls
lkwinta Aug 6, 2025
9018a3c
typo
lkwinta Aug 6, 2025
e0c6311
Merge branch 'main' into file_packager_errback
lkwinta Aug 6, 2025
fb0be1e
ruff fix
lkwinta Aug 6, 2025
19d1d53
address @sbc100 review
lkwinta Aug 6, 2025
19df551
unify rejecting promises
lkwinta Aug 6, 2025
633b44e
throw Error instead of promise reject
lkwinta Aug 6, 2025
f566133
fix pipeline
lkwinta Aug 6, 2025
87e45dc
Address @sbc100 review
lkwinta Aug 7, 2025
9e11b66
make sure to brake promise chain while error on fetching data
lkwinta Aug 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 23 additions & 14 deletions tools/file_packager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,12 @@ def generate_js(data_target, data_files, metadata):

if options.support_node:
ret += " var isNode = typeof process === 'object' && typeof process.versions === 'object' && typeof process.versions.node === 'string';\n"
ret += ' function loadPackage(metadata) {\n'
ret += '''
function throwPackageError(error) {
throw error;
};

function loadPackage(metadata) {\n'''

code = '''
function assert(check, msg) {
Expand Down Expand Up @@ -972,20 +977,25 @@ def generate_js(data_target, data_files, metadata):
%(node_support_code)s
Module['dataFileDownloads'] ??= {};
fetch(packageName)
.catch((cause) => Promise.reject(new Error(`Network Error: ${packageName}`, {cause}))) // If fetch fails, rewrite the error to include the failing URL & the cause.
.catch((cause) => {
errback(new Error(`Network Error: ${packageName}`, {cause}));
// We want to be sure to cancel promise chain because we sometimes call this function with method
// which doesn't throw error hance promise chain is not broken
return Promise.reject(new Error(`Network Error: ${packageName}`, {cause}));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is starting to look pretty ugly because we mixing callbacks and promises .. I think converting to using async await is ultimately what we we probably want to do here.

Copy link
Contributor Author

@lkwinta lkwinta Aug 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have prototyped something, but maybe it deservse separate PR...

function fetchRemotePackage(packageName, packageSize) {
        return new Promise((fetchPackageResolve, fetchPackageReject) => {
          if (isNode) {
          require('fs').readFile(packageName, (err, contents) => {
            if (err) {
              fetchPackageReject(err);
            } else {
              fetchPackageResolve(contents.buffer);
            }
          });
          return;
        }
          Module['dataFileDownloads'] ??= {};
          fetch(packageName)
            .catch((cause) => {
              err = new Error(`Network Error: ${packageName}`, {cause});
              fetchPackageReject(err);
              return Promise.reject(err);
            }) // If fetch fails, rewrite the error to include the failing URL & the cause.
            .then((response) => {
              if (!response.ok) {
                fetchPackageReject(new Error(`${response.status}: ${response.url}`));
                return;
              }

              if (!response.body && response.arrayBuffer) { // If we're using the polyfill, readers won't be available...
                return response.arrayBuffer().then(fetchPackageResolve);
              }

              const reader = response.body.getReader();
              const iterate = () => reader.read().then(handleChunk).catch((cause) => {
                fetchPackageReject(new Error(`Unexpected error while handling : ${response.url} ${cause}`, {cause}));
                return;
              });

              const chunks = [];
              const headers = response.headers;
              const total = Number(headers.get('Content-Length') ?? packageSize);
              let loaded = 0;

              const handleChunk = ({done, value}) => {
                if (!done) {
                  chunks.push(value);
                  loaded += value.length;
                  Module['dataFileDownloads'][packageName] = {loaded, total};

                  let totalLoaded = 0;
                  let totalSize = 0;

                  for (const download of Object.values(Module['dataFileDownloads'])) {
                    totalLoaded += download.loaded;
                    totalSize += download.total;
                  }

                  Module['setStatus']?.(`Downloading data... (${totalLoaded}/${totalSize})`);
                  return iterate();
                } else {
                  const packageData = new Uint8Array(chunks.map((c) => c.length).reduce((a, b) => a + b, 0));
                  let offset = 0;
                  for (const chunk of chunks) {
                    packageData.set(chunk, offset);
                    offset += chunk.length;
                  }
                  fetchPackageResolve(packageData.buffer);
                }
              };

              Module['setStatus']?.('Downloading data...');
              return iterate();
            });
        });
      };
      ```

}) // If fetch fails, rewrite the error to include the failing URL & the cause.
.then((response) => {
if (!response.ok) {
return Promise.reject(new Error(`${response.status}: ${response.url}`));
return errback(new Error(`${response.status}: ${response.url}`));
}

if (!response.body && response.arrayBuffer) { // If we're using the polyfill, readers won't be available...
return response.arrayBuffer().then(callback);
}

const reader = response.body.getReader();
const iterate = () => reader.read().then(handleChunk).catch((cause) => {
return Promise.reject(new Error(`Unexpected error while handling : ${response.url} ${cause}`, {cause}));
});
const iterate = () => reader.read().then(handleChunk).catch((cause) =>
errback(new Error(`Unexpected error while handling : ${response.url} ${cause}`, {cause}))
);

const chunks = [];
const headers = response.headers;
Expand Down Expand Up @@ -1022,10 +1032,6 @@ def generate_js(data_target, data_files, metadata):
Module['setStatus']?.('Downloading data...');
return iterate();
});
};

function handleError(error) {
console.error('package error:', error);
};\n''' % {'node_support_code': node_support_code}

code += '''
Expand All @@ -1048,7 +1054,7 @@ def generate_js(data_target, data_files, metadata):
function preloadFallback(error) {
console.error(error);
console.error('falling back to default preload behavior');
fetchRemotePackage(REMOTE_PACKAGE_NAME, REMOTE_PACKAGE_SIZE, processPackageData, handleError);
fetchRemotePackage(REMOTE_PACKAGE_NAME, REMOTE_PACKAGE_SIZE, processPackageData, throwPackageError);
};

openDatabase(
Expand Down Expand Up @@ -1088,7 +1094,7 @@ def generate_js(data_target, data_files, metadata):
} else {
fetched = data;
}
}, handleError);\n'''
}, throwPackageError);\n'''

code += '''
Module['preloadResults'][PACKAGE_NAME] = {fromCache: false};
Expand Down Expand Up @@ -1117,7 +1123,7 @@ def generate_js(data_target, data_files, metadata):
if (isNode) {
require('fs').readFile(metadataUrl, 'utf8', (err, contents) => {
if (err) {
return Promise.reject(err);
throwPackageError(err);
} else {
loadPackage(JSON.parse(contents));
}
Expand All @@ -1134,11 +1140,14 @@ def generate_js(data_target, data_files, metadata):
var metadataUrl = Module['locateFile'] ? Module['locateFile']('%(metadata_file)s', '') : '%(metadata_file)s';
%(node_support_code)s
fetch(metadataUrl)
.catch((cause) =>
throwPackageError(new Error(`Network Error: ${packageName}`, {cause}))
) // If fetch fails, rewrite the error to include the failing URL & the cause.
.then((response) => {
if (response.ok) {
return response.json();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you still need to do something here when the response is not ok... you either need the throw or return a rejected promise object I think.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actaully I think just doing throwPackageError here on this line is probably what you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder now, if we take this code:

function preloadFallback(error) {
          console.error(error);
          console.error('falling back to default preload behavior');
          fetchRemotePackage(REMOTE_PACKAGE_NAME, REMOTE_PACKAGE_SIZE, processPackageData, throwPackageError);
        };

        openDatabase(
          (db) => checkCachedPackage(db, PACKAGE_PATH + PACKAGE_NAME,
              (useCached, metadata) => {
                Module['preloadResults'][PACKAGE_NAME] = {fromCache: useCached};
                if (useCached) {
                  fetchCachedPackage(db, PACKAGE_PATH + PACKAGE_NAME, metadata, processPackageData, preloadFallback);
                } else {
                  fetchRemotePackage(REMOTE_PACKAGE_NAME, REMOTE_PACKAGE_SIZE,
                    (packageData) => {
                      cacheRemotePackage(db, PACKAGE_PATH + PACKAGE_NAME, packageData, {uuid:PACKAGE_UUID}, processPackageData,
                        (error) => {
                          console.error(error);
                          processPackageData(packageData);
                        });
                    }
                  , preloadFallback);
                }
              }, preloadFallback)
        , preloadFallback);

then this catch in fetchRemotePackage will not fail

 Module['dataFileDownloads'] ??= {};
        fetch(packageName)
          .catch((cause) => errback(new Error(`Network Error: ${packageName}`, {cause}))) // If fetch fails, rewrite the error to include the failing URL & the cause.
          .then((response) => {

I think that just adding promise Rejsect here for security iss what we want

return Promise.reject(new Error(`${response.status}: ${response.url}`));
throwPackageError(new Error(`Response error: ${response}`));
})
.then(loadPackage);
}
Expand Down