-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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.
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
fix: top-level-await module execution #7946
Conversation
I think I hit an edge case where trying #7907 on this branch - there's most likely an interval being set up by one of submodules which halts execution of one of dynamic imports. I need to think a bit more how that should be handled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I also confirmed it doesn't have a hot loop when idling by running
cargo run -- run --allow-net https://deno.land/[email protected]/examples/echo_server.ts
So I've managed to find one more bug in implementation which is directly related to #7945 // a.mjs
import { foo } from "./timeout_loop.mjs";
export const collection = [];
const mod = await import("./b.mjs");
console.error("foo in main", foo);
console.error("mod", mod); // b.mjs
import { foo } from "./timeout_loop.mjs";
import { collection } from "./a.mjs";
console.log("collection in b", collection);
console.log("foo in b", foo);
export const a = "a"; // timeout_loop.js
export const foo = "foo";
export function delay(ms) {
return new Promise((res) =>
setTimeout(() => { res(); }, ms)
);
}
let i = 0;
async function timeoutLoop() {
await delay(1000);
console.log("timeout loop", i);
i++;
if (i > 5) {
return;
}
timeoutLoop();
}
timeoutLoop();
Node exits with code 13
|
); | ||
let promise = v8::Local::<v8::Promise>::try_from(value) | ||
.expect("Expected to get promise as module evaluation result"); | ||
let promise_id = promise.get_identity_hash(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be problematic. The hash is only a 22 bits random number so, per the Birthday Paradox, there's a 50% chance of a collision after ~2400 turns.
It's a bit (hah!) worse on 32 bits architectures because the hash is one bit smaller, it reaches 50% after ~1700 turns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we already use this method for other purposes and have been for quite some time - what would you suggest to do instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do it with modules too - and that should also change.
I'd assign the promise a number and stick it in a HashMap<u64, v8::Global<v8::Promise>>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably need to somehow get back the id when you have a bare promise, I suppose?
It could be stored on the promise itself with a v8::Private
symbol but that requires rusty_v8 to grow bindings for v8::Object::SetPrivate()
and v8::Object::GetPrivate()
. I can add those, I had that on my todo list anyway.
However, that won't work for modules because they're instances of v8::Data
, not v8::Object
. What could work is storing them in a HashSet
and implement Eq
and Hash
ourselves.
hash()
could use module.get_identity_hash()
(duplicates are okay) and eq()
could compare the script id. There's currently no binding for that but it's trivial to add.
(edit: on second thought, the script id is unique, it can just be used as the key in a HashMap
.)
Same approach should work for promises, but using strict_equals()
in eq()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for suggestions - I think we could land this PR as is and I'll update whole deno_core
to this new mechanism in the follow up PR
Currently failing, but making this test case pass will ensure our TLA implementation is correct.
Second try for #7672
Reverted in #7911 because of #7909 (which has been addressed in this PR).
Fixes #7907
Fixes #7649
Fixes #7945