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

lkwinta · 2025-08-06T18:29:33Z

errback argument was ignored in fetchRemotePackage, Promise.reject was called instead

lkwinta · 2025-08-06T18:29:40Z

sbc100 · 2025-08-06T18:43:05Z

By the way it looks like this code was added in #22016, but the code in replaced also did throw new Error rather than calling errback.

Also, it looks like errback is just doing console.error('package error:', error); .. not sure thats any better.

This change does seem perfectly good, but I'm wondoring how it solves your issue if the result is just to call console.error?

lkwinta · 2025-08-06T18:47:13Z

By the way it looks like this code was added in #22016, but the code in replaced also did throw new Error rather than calling errback.

Also, it looks like errback is just doing console.error('package error:', error); .. not sure thats any better.

This change does seem perfectly good, but I'm wondoring how it solves your issue if the result is just to call console.error?

I will now have one place where I can reject the promise returned by function.

Other thing that comes to my mind is changing console.errors in whole script to Module.printErr to enable filtering or guard the logs with some flag

lkwinta · 2025-08-06T19:09:38Z

@sbc100 I think that I have more proposal as part of this mr

sbc100 · 2025-08-06T20:03:53Z

tools/file_packager.py

+  ret += ''' 
+    function handleError(error) {
+      console.error('package error:', error);
+      return error;


Why add return error here?

I think think about it now again it is really unnecessary

tools/file_packager.py

sbc100 · 2025-08-06T20:09:33Z

tools/file_packager.py

@@ -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);


I think you can delete the return keyword here.

lkwinta · 2025-08-06T20:17:58Z

Should I merge when the pipeline passes now?

tools/file_packager.py

lkwinta · 2025-08-07T09:00:03Z

@sbc100 I have fixed the pipeline, some returns where neccessary since errback had different meaning, I wonder if we should modify catch statement in fetchRemotePackage now

sbc100 · 2025-08-07T17:20:37Z

tools/file_packager.py

@@ -984,7 +989,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}));


I don't think this return is needed here.

sbc100 · 2025-08-07T17:22:10Z

tools/file_packager.py

@@ -1138,8 +1139,10 @@ def generate_js(data_target, data_files, metadata):
        if (response.ok) {
          return response.json();
        }


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.

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

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

sbc100 · 2025-08-07T22:17:29Z

tools/file_packager.py

+            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}));


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.

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(); }); }); }; ```

sbc100 · 2025-08-07T22:53:06Z

I took as stab as converting part of the file_packager output to async/await: #24871. Its not the relevant part .. but its a start.

lkwinta · 2025-08-07T22:58:19Z

I took as stab as converting part of the file_packager output to async/await: #24871. Its not the relevant part .. but its a start.

So should I fix that now ?

sbc100 · 2025-08-07T23:04:11Z

I took as stab as converting part of the file_packager output to async/await: #24871. Its not the relevant part .. but its a start.

So should I fix that now ?

I've made some progress on it, so can do this part if you like?

sbc100 · 2025-08-07T23:05:02Z

I took as stab as converting part of the file_packager output to async/await: #24871. Its not the relevant part .. but its a start.

So should I fix that now ?

I've made some progress on it, so can do this part if you like?

Ooops that should have been: #24882

lkwinta · 2025-08-07T23:12:40Z

I took as stab as converting part of the file_packager output to async/await: #24871. Its not the relevant part .. but its a start.

So should I fix that now ?

I've made some progress on it, so can do this part if you like?

Not right now for sure, but feel free to take and use my prototype, I have changed it now after some tests, at least happy path should pass with current version pasted in my comment.

sbc100 · 2025-08-07T23:18:25Z

I think I'm going to go for full conversion to async/await which should make your change much simpler (I hope)

sbc100 · 2025-08-07T23:39:22Z

Part 2: #24883

lkwinta · 2025-08-08T06:08:45Z

I think I'm going to go for full conversion to async/await which should make your change much simpler (I hope)

So we wait with this PR and the export_es6 one for the conversion to async?

sbc100 · 2025-08-08T14:49:15Z

Yes I think the async conversion work will supersede this PR

lkwinta · 2025-08-08T21:26:15Z

Yes I think the async conversion work will supersede this PR

@sbc100 to be honest I don't see the point of this PR anymore after your changes. I think it will be possible to Reject main functions promise from just one or two places without the need of handleError

sbc100 · 2025-08-08T21:35:55Z

Yes I think the async conversion work will supersede this PR

@sbc100 to be honest I don't see the point of this PR anymore after your changes. I think it will be possible to Reject main functions promise from just one or two places without the need of handleError

That sounds reasonable to me.

The non-es6 case I think the point of handleError is to show something on the console to the user so they know the package loading has failed.

For the es6 case i don't think that is necessary, so I think you could just do handleError = reject maybe?

lkwinta · 2025-08-08T21:38:25Z

Yes I think the async conversion work will supersede this PR

@sbc100 to be honest I don't see the point of this PR anymore after your changes. I think it will be possible to Reject main functions promise from just one or two places without the need of handleError

That sounds reasonable to me.

The non-es6 case I think the point of handleError is to show something on the console to the user so they know the package loading has failed.

For the es6 case i don't think that is necessary, so I think you could just do handleError = reject maybe?

So I think I can close that

sbc100 · 2025-08-08T21:39:29Z

Yes, I think this PR can be closed. Is that what you are asking? I think the conversion async removed the need for this fix.

[file_packager.py] Fix errback in fetchRemotePackage

a423463

lkwinta mentioned this pull request Aug 6, 2025

[file_packager.py] Add --export-es6 to file_packager.py #24737

Merged

sbc100 approved these changes Aug 6, 2025

View reviewed changes

Merge branch 'main' into file_packager_errback

36a127e

lkwinta added 2 commits August 6, 2025 21:14

Fix all Promise.reject calls

72abcbd

typo

9018a3c

lkwinta requested a review from sbc100 August 6, 2025 19:17

lkwinta added 2 commits August 6, 2025 21:54

Merge branch 'main' into file_packager_errback

e0c6311

ruff fix

fb0be1e

sbc100 reviewed Aug 6, 2025

View reviewed changes

address @sbc100 review

19d1d53

sbc100 reviewed Aug 6, 2025

View reviewed changes

tools/file_packager.py Show resolved Hide resolved

lkwinta added 2 commits August 6, 2025 20:39

unify rejecting promises

19df551

throw Error instead of promise reject

633b44e

lkwinta requested a review from sbc100 August 6, 2025 21:05

sbc100 approved these changes Aug 6, 2025

View reviewed changes

sbc100 enabled auto-merge (squash) August 6, 2025 21:15

auto-merge was automatically disabled August 6, 2025 22:02
Head branch was pushed to by a user without write access

fix pipeline

f566133

lkwinta force-pushed the file_packager_errback branch from d9001d8 to f566133 Compare August 7, 2025 07:41

sbc100 reviewed Aug 7, 2025

View reviewed changes

lkwinta added 2 commits August 7, 2025 20:58

Address @sbc100 review

87e45dc

make sure to brake promise chain while error on fetching data

9e11b66

lkwinta requested a review from sbc100 August 7, 2025 21:06

sbc100 reviewed Aug 7, 2025

View reviewed changes

lkwinta closed this Aug 8, 2025

lkwinta deleted the file_packager_errback branch August 8, 2025 21:39

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

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

Uh oh!

Conversation

lkwinta commented Aug 6, 2025

Uh oh!

lkwinta commented Aug 6, 2025

Uh oh!

sbc100 commented Aug 6, 2025

Uh oh!

lkwinta commented Aug 6, 2025

Uh oh!

lkwinta commented Aug 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lkwinta commented Aug 6, 2025

Uh oh!

Uh oh!

lkwinta commented Aug 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lkwinta Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Aug 7, 2025

Uh oh!

lkwinta commented Aug 7, 2025

Uh oh!

sbc100 commented Aug 7, 2025

Uh oh!

sbc100 commented Aug 7, 2025

Uh oh!

lkwinta commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Aug 7, 2025

Uh oh!

sbc100 commented Aug 7, 2025

Uh oh!

lkwinta commented Aug 8, 2025

Uh oh!

sbc100 commented Aug 8, 2025

Uh oh!

lkwinta commented Aug 8, 2025

Uh oh!

sbc100 commented Aug 8, 2025

Uh oh!

lkwinta commented Aug 8, 2025

Uh oh!

sbc100 commented Aug 8, 2025

Uh oh!

Uh oh!

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

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

lkwinta Aug 7, 2025 •

edited

Loading

lkwinta commented Aug 7, 2025 •

edited

Loading