Improve error for fetch on top-level with dedicated snapshot#6654
Improve error for fetch on top-level with dedicated snapshot#6654
fetch on top-level with dedicated snapshot#6654Conversation
…error Changes error for top level fetch from: ``` TypeError: Illegal invocation: function called with incorrect `this` reference. ``` to: ``` Error: Disallowed operation called within global scope. Asynchronous I/O (ex: fetch() or connect()), setting a timeout, and generating random values are not allowed within global scope. ... ``` This is a workaround for the fact that we don't have pyodide/pyodide#5588 in Pyodide 0.28.2, this fix is upstreamed starting with Pyodide 0.29.0
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This defers JS module imports from module evaluation time to setDoAnImport() time, and wraps globalThis with createImportProxy so that top-level fetch() calls produce the helpful "Disallowed operation" error instead of the confusing "Illegal invocation" error during snapshot creation.
Issues (by severity):
- Medium --
'global this'sentinel causes broken deserialization. InimportJsModulesFromSnapshot_,doImport('global this')returns the string'global this'rather thanglobalThis. If a dedicated snapshot serialized a reference to e.g.globalThis.fetch, thendeserializeJsModulewould try'global this'['fetch']which isundefined. This is a latent bug that would surface if_makeSnapshotis re-enabled without fixing this. Since the comment at line 936 says "We can reenable this with Pyodide 0.29.0 and later", this will bite the next person who flips it back on.
| } | ||
| async function doImport(x: string): Promise<any> { | ||
| if (x === 'global this') { | ||
| return 'global this'; |
There was a problem hiding this comment.
Returning the string 'global this' means deserializeJsModule will try to walk accessor lists on a string (e.g., 'global this'['fetch'] → undefined). This is harmless today because _makeSnapshot is disabled, but it's a latent bug that will silently break when that flag is re-enabled for Pyodide 0.29.0.
Consider returning the actual globalThis so deserialization works correctly:
| return 'global this'; | |
| return globalThis; |
|
I'm Bonk, and I've done a quick review of your PR. This defers JS module imports from module evaluation time to Issues (by severity):
Review posted to PR #6654. |
Merging this PR will not alter performance
Comparing 
Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6654 +/- ##
==========================================
- Coverage 70.68% 66.52% -4.17%
==========================================
Files 420 405 -15
Lines 113010 115971 +2961
Branches 18531 19408 +877
==========================================
- Hits 79881 77144 -2737
- Misses 22086 27244 +5158
- Partials 11043 11583 +540 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dom96
left a comment
dom96
left a comment
There was a problem hiding this comment.
Could we get a test for this?
| customSerializedObjects: CustomSerializedObjects | ||
| ): Pyodide { | ||
| try { | ||
| const Module = enterJaegerSpan('instantiate_emscripten', () => | ||
| SetupEmscripten.getModule() | ||
| ); | ||
| Module.compileModuleFromReadOnlyFS = compileModuleFromReadOnlyFS; | ||
| Module.API.config.jsglobals = globalThis; | ||
| // Next line is not needed with Pyodide >= 0.29.0 |
There was a problem hiding this comment.
Should you add a version check here?
| ); | ||
| let JS_MODULES: Record<string, any>; | ||
|
|
||
| export async function importJsModulesFromSnapshot( |
There was a problem hiding this comment.
This indirection is unfortunate, anything we can do to fix it? I'd say at least give this function a different name:
| export async function importJsModulesFromSnapshot( | |
| export async function fillSnapshotJsModules( |
| // We can reenable this with Pyodide 0.29.0 and later. | ||
| // Module.API.config._makeSnapshot = | ||
| // IS_CREATING_SNAPSHOT && Module.API.version !== PyodideVersion.V0_26_0a2; |
There was a problem hiding this comment.
Could you just add a version check here instead of disabling it completely?
3 participants
Changes error for top level fetch from:
to:
This is a workaround for the fact that we don't have pyodide/pyodide#5588 in Pyodide 0.28.2, this fix is upstreamed starting with Pyodide 0.29.0