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
+23 −14
Closed

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

Changes from 5 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
22 changes: 12 additions & 10 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,13 @@ 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 handleError(error) {
console.error('package error:', error);
return error;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why add return error here?

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 think think about it now again it is really unnecessary

};

function loadPackage(metadata) {\n'''

code = '''
function assert(check, msg) {
Expand Down Expand Up @@ -972,10 +978,10 @@ 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}))) // 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...
Expand All @@ -984,7 +990,7 @@ def generate_js(data_target, data_files, metadata):

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}));
return errback(new Error(`Unexpected error while handling : ${response.url} ${cause}`, {cause}));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this return is needed here.

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 agree,

});

const chunks = [];
Expand Down Expand Up @@ -1022,10 +1028,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 Down Expand Up @@ -1117,7 +1119,7 @@ def generate_js(data_target, data_files, metadata):
if (isNode) {
require('fs').readFile(metadataUrl, 'utf8', (err, contents) => {
if (err) {
return Promise.reject(err);
return handleError(err);
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 can delete the return keyword here.

} else {
loadPackage(JSON.parse(contents));
}
Expand All @@ -1138,7 +1140,7 @@ def generate_js(data_target, data_files, metadata):
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}`));
return handleError(new Error(`${response.status}: ${response.url}`));
})
.then(loadPackage);
}
Expand Down