Use `fetch()` for loading packages & shared libraries #22002

seanmorris · 2024-05-26T01:50:43Z

This PR switches AJAX requests for file packages & shared libraries to be loaded with fetch() rather than XmlHttpRequest(). This allows service workers to make AJAX requests, since XmlHttpRequest is not available in service workers.

Previously, --embed-file was necessary to load file packages, which embeds the files into the binary and precludes caching and re-use of the package. Loading shared libraries is not possible in a service worker without this patch.

Fixes #22003

…to sm-worker-fetch

sbc100

I wonder if we need a new option at all here or if we should just do this in all cases?

sbc100 · 2024-05-28T16:47:11Z

src/web_or_worker_shell_read.js

+    fetch(url)
+    .then(response => {
+      if(response.ok) {
+        return response.arrayBuffer();


Why not call onload here?

response.arrayBuffer(); returns a promise that resolves to a buffer, not a literal buffer.

sbc100 · 2024-05-28T16:47:14Z

src/web_or_worker_shell_read.js

+      if(response.ok) {
+        return response.arrayBuffer();
+      }
+      throw new Error(response.statusText + ' : ' + response.url);


Should this not call onerror instead?

Let me check the make sure the error has the correct stack if its not actually thrown, and then I can change it.

I remembered why I used this pattern as I was editing: The catch() at the end of the function will catch errors coming from the fetch(), but fetch() won't throw errors by itself if the request fails, i.e. in a 404 or 500 error. For that we have to throw our own error. When we do that, we skip the then() on line 32 and head right to catch().

How about return new Promise.reject(..)

Then I think maybe you can do .then(onload, onerror) without the extra catch? Not sure which is more idomatic.

seanmorris · 2024-05-28T19:32:21Z

I wonder if we need a new option at all here or if we should just do this in all cases?

I'd like this to be enabled by default, but I don't want to introduce any surprises in cases where people are polyfilling/overriding XmlHttpRequest. Perhaps we can enable it by default in the next minor release, and eventually deprecate the use of XmlHttpRequest gracefully.

sbc100 · 2024-05-28T19:49:03Z

I wonder if we need a new option at all here or if we should just do this in all cases?

I'd like this to be enabled by default, but I don't want to introduce any surprises in cases where people are polyfilling/overriding XmlHttpRequest. Perhaps we can enable it by default in the next minor release, and eventually deprecate the use of XmlHttpRequest gracefully.

We will also likely need to write a minimal polyfil for cases where we target older browser. (i.e. -sLEGACY_BROWSER_SUPPORT). See the existing polyfills in src/polyfill.

sbc100 · 2024-05-28T19:52:04Z

I wonder if we need a new option at all here or if we should just do this in all cases?

I'd like this to be enabled by default, but I don't want to introduce any surprises in cases where people are polyfilling/overriding XmlHttpRequest. Perhaps we can enable it by default in the next minor release, and eventually deprecate the use of XmlHttpRequest gracefully.

Polyfilling/overriding XMLHttpRequest is not something that I know of anyone doing and we don't document emscripten as supporting this. I think its reasonable to simply make the switch and add a ChangeLog entry for this.. if somebody really is overriding XMLHttpRequest it should be easy enough to also override fetch. That was we can avoid adding a new setting (each time we do this it increases our testing burden).

seanmorris · 2024-05-28T20:16:56Z

I wonder if we need a new option at all here or if we should just do this in all cases?

I'd like this to be enabled by default, but I don't want to introduce any surprises in cases where people are polyfilling/overriding XmlHttpRequest. Perhaps we can enable it by default in the next minor release, and eventually deprecate the use of XmlHttpRequest gracefully.

Polyfilling/overriding XMLHttpRequest is not something that I know of anyone doing and we don't document emscripten as supporting this. I think its reasonable to simply make the switch and add a ChangeLog entry for this.. if somebody really is overriding XMLHttpRequest it should be easy enough to also override fetch. That was we can avoid adding a new setting (each time we do this it increases our testing burden).

I'll go ahead and make those changes.

This reverts commit 0bf9b71.

Merging main

sbc100 · 2024-05-29T13:18:47Z

If possible, I'd like to see the fetchAsync change land first and the file_packager change as followup.

The fetchAsync is going to be a lot more risky, since it effects basically all users whereas the file packager change is something that only some users will be effected by.

sbc100 · 2024-05-29T13:22:58Z

tools/file_packager.py

+        Module.dataFileDownloads = Module.dataFileDownloads || {};
+        const url = packageName;
+        fetch(url)
+        .catch(error => { throw new Error(error + ' : ' + url) }) // Do this first so we don't catch errors from then()


Instead of .catch here is it the same to write the error handler as the second argument to then()? Is one way preferable over the other?

In this case, we need to catch() before then() so we only handle fetch errors here. If we putt this at the bottom, or in the second param of this(), then it will also act on the network errors generated by then(), and I'd need to add logic to skip certain errors.

sbc100 · 2024-05-29T13:23:22Z

tools/file_packager.py

+        Module.dataFileDownloads = Module.dataFileDownloads || {};
+        const url = packageName;
+        fetch(url)
+        .catch(error => { throw new Error(error + ' : ' + url) }) // Do this first so we don't catch errors from then()


I think our style uses braces around the single argument to arrow functions

throw isn't a function, so it needs to have braces or it counts as a syntax error. I'm switching to Promise.reject which is a function.

sbc100 · 2024-05-29T13:27:52Z

tools/file_packager.py

+      if(response.ok) {
+        return response.json();
+      }
+      throw new Error(response.statusText + ' : ' + response.url);


Would return Promise.reject(new Error(response.statusText + ' : ' + response.url)) instead of throwing?

seanmorris · 2024-05-30T04:06:02Z

If possible, I'd like to see the fetchAsync change land first and the file_packager change as followup.

The fetchAsync is going to be a lot more risky, since it effects basically all users whereas the file packager change is something that only some users will be effected by.

#22015 - libs
#22016 - packages

Let me know how those look.

sbc100 · 2024-06-03T15:22:37Z

Now that #22015 and #22016 contains the rest of this PR I think we can close this one?

seanmorris added 8 commits May 25, 2024 11:23

Adding USE_FETCH setting.

87743d1

Tweaks

3d38841

Adding tests.

ffcaa3b

Tweak.

de79ead

Tweak.

139bcd4

Merge branch 'main' into sm-worker-fetch

893f361

Tweak.

0407112

Merge branch 'sm-worker-fetch' of github.com:seanmorris/emscripten in…

24761b6

…to sm-worker-fetch

seanmorris mentioned this pull request May 26, 2024

Can't preload files/shared libs in service worker due to lack of XHR [PR OPEN] #22003

Closed

seanmorris added 11 commits May 25, 2024 22:43

Rebasing generated sizes.

3ead8de

Reverting extraneous change.

35b593f

Readability.

5c2a7bb

Testing

5631928

Removing debug.

ff2d142

Typo.

9e06ef2

Parameterizing test.

5899d3f

Spacing.

c09f2c5

Deduping default info in docs.

2f43c20

Deduping if statement.

c5ecce5

Correcting indentation of JS output.

7e86b5b

sbc100 reviewed May 28, 2024

View reviewed changes

Removing use_fetch option and XmlHttpRequest.

6848055

seanmorris changed the title ~~Adding -sUSE_FETCH for loading packages & shared libraries in service workers~~ Use fetch for loading packages & shared libraries May 29, 2024

seanmorris changed the title ~~Use fetch for loading packages & shared libraries~~ Use fetch() for loading packages & shared libraries May 29, 2024

seanmorris added 2 commits May 29, 2024 03:10

Reverting extraneous changes.

0bf9b71

Revert "Reverting extraneous changes."

3f765bc

This reverts commit 0bf9b71.

seanmorris added 6 commits May 29, 2024 03:13

Actually reverting extraneous change.

316aa8b

Merge branch 'sm-worker-fetch' into updates

366b942

Merging main

58ab5c6

Merging main

Throw error on bad fetch in runMetaWithFS

5899e42

Tweaks.

92ffd13

Tweaks.

df4e1c3

sbc100 reviewed May 29, 2024

View reviewed changes

seanmorris added 11 commits May 29, 2024 13:19

Use rejecting promises instead of throw.

bb9a69e

Merge branch 'main' into sm-worker-fetch

c8e9db9

Tweaks.

272f093

Adding polyfill + test.

6432c5a

Adding polyfill + test.

e870667

Adding polyfill + test.

ce4089e

Adding fetch polyfill + test

4581d90

Adding fetch polyfill + test

457ef2c

Polyfill tests for browser.

aeac94e

Tweaks.

275388a

Tweaks.

d4a6162

sbc100 closed this Jun 6, 2024

Use fetch() for loading packages & shared libraries #22002

Use fetch() for loading packages & shared libraries #22002

Uh oh!

Conversation

seanmorris commented May 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 left a comment

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!

seanmorris May 29, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

seanmorris commented May 28, 2024

Uh oh!

sbc100 commented May 28, 2024

Uh oh!

sbc100 commented May 28, 2024

Uh oh!

seanmorris commented May 28, 2024

Uh oh!

sbc100 commented May 29, 2024

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!

seanmorris commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Jun 3, 2024

Uh oh!

Uh oh!

Use `fetch()` for loading packages & shared libraries #22002

Use `fetch()` for loading packages & shared libraries #22002

seanmorris commented May 26, 2024 •

edited

Loading

seanmorris May 29, 2024 •

edited

Loading

seanmorris commented May 30, 2024 •

edited

Loading