Skip to content

Commit aa7e4c7

Browse files
vados-cosmonicvados-cosmonic
committed
fix(bindgen): re-enable determinism coinflip
This commit enables a bunch of changes that are made more visible with randomness enabled -- exposing some ordering bugs with async tasks that were racing to complete. - fix async concurrent operation gating check - fix task waiting logic - stronger checks around waitable set injection - separate waitUntil impls - disable ad hoc task waiting code - locking & releasing of components - global current task usage - use uwriteln in more places, remove stale code
1 parent 44700fe commit aa7e4c7

5 files changed

Lines changed: 322 additions & 166 deletions

File tree

‎crates/js-component-bindgen/src/function_bindgen.rs‎

Lines changed: 10 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -391,31 +391,8 @@ impl FunctionBindgen<'_> {
391391
let start_current_task_fn = self.intrinsic(Intrinsic::AsyncTask(
392392
AsyncTaskIntrinsic::CreateNewCurrentTask,
393393
));
394-
let global_task_map = self.intrinsic(Intrinsic::AsyncTask(
395-
AsyncTaskIntrinsic::GlobalAsyncCurrentTaskMap,
396-
));
397394
let component_instance_idx = self.canon_opts.instance.as_u32();
398395

399-
// If we're within an async function, wait for all top level previous tasks to finish before running
400-
// to ensure that guests do not try to run two tasks at the same time.
401-
if is_async && self.requires_async_porcelain {
402-
uwriteln!(
403-
self.src,
404-
r#"
405-
// All other tasks must finish before we can start this one
406-
const taskMetas = {global_task_map}.get({component_instance_idx});
407-
if (taskMetas) {{
408-
const taskPromises = taskMetas
409-
.filter(mt => mt.componentIdx === {component_instance_idx})
410-
.map(mt => mt.task)
411-
.filter(t => !t.getParentSubtask())
412-
.map(t => t.exitPromise());
413-
await Promise.allSettled(taskPromises);
414-
}}
415-
"#,
416-
);
417-
}
418-
419396
uwriteln!(
420397
self.src,
421398
r#"
@@ -1550,6 +1527,11 @@ impl Bindgen for FunctionBindgen<'_> {
15501527
self.intrinsic(Intrinsic::AsyncTask(AsyncTaskIntrinsic::GetCurrentTask));
15511528
let component_instance_idx = self.canon_opts.instance.as_u32();
15521529

1530+
// At first, use the global current task metadata, in case we are executing from
1531+
// inside a with-global-current-task wrapper
1532+
let get_global_current_task_meta_fn =
1533+
self.intrinsic(Intrinsic::GetGlobalCurrentTaskMetaFn);
1534+
15531535
uwriteln!(
15541536
self.src,
15551537
"{debug_log_fn}('{prefix} [Instruction::CallInterface] ({async_}, @ enter)');",
@@ -1604,7 +1586,11 @@ impl Bindgen for FunctionBindgen<'_> {
16041586
}};
16051587
16061588
taskCreation: {{
1607-
parentTask = {current_task_get_fn}({component_instance_idx})?.task;
1589+
parentTask = {current_task_get_fn}(
1590+
{component_instance_idx},
1591+
{get_global_current_task_meta_fn}({component_instance_idx})?.taskID,
1592+
)?.task;
1593+
16081594
if (!parentTask) {{
16091595
createTask();
16101596
break taskCreation;

‎crates/js-component-bindgen/src/intrinsics/component.rs‎

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -303,8 +303,6 @@ impl ComponentIntrinsic {
303303
this.#locked = locked;
304304
}}
305305
306-
// TODO(fix): we might want to check for pre-locked status here, we should be deterministically
307-
// going from locked -> unlocked and vice versa
308306
exclusiveLock() {{
309307
{debug_log_fn}('[{component_async_state_class}#exclusiveLock()]', {{
310308
locked: this.#locked,
@@ -337,6 +335,54 @@ impl ComponentIntrinsic {
337335
this.#onExclusiveReleaseHandlers.push(fn);
338336
}}
339337
338+
// nextTaskPromise & nextTaskQueue are used to await current task completion and queues
339+
// any tasks attempting to enter() and complete.
340+
//
341+
// see: nextTaskExecutionSlot()
342+
//
343+
// TODO(threads): this should be unnecessary once threads are properly implemented,
344+
// as the task.enter() logic should suffice (it should be guaranteed that we cannot re-enter
345+
// unless the task in question is the current task in the thread execution, and only one can
346+
// run at a time)
347+
#nextTaskPromise = Promise.resolve(true);
348+
#nextTaskQueue = [];
349+
350+
async nextTaskExecutionSlot(args) {{
351+
const {{ task }} = args;
352+
353+
const placeholder = {{
354+
completed: false,
355+
task,
356+
promise: task.exitPromise().then(() => {{
357+
placeholder.completed = true;
358+
}}),
359+
}};
360+
this.#nextTaskQueue.push(placeholder);
361+
362+
let next;
363+
while (true) {{
364+
await this.#nextTaskPromise;
365+
366+
next = this.#nextTaskQueue.find(placeholder => !placeholder.completed);
367+
368+
// This task is next in the queue, we can continue
369+
if (next === undefined || next === placeholder) {{
370+
this.#nextTaskPromise = next.promise;
371+
if (this.#nextTaskQueue.length > 1000) {{
372+
this.#nextTaskQueue = this.#nextTaskQueue.filter(p => !p.completed);
373+
if (this.#nextTaskQueue.length > 1000) {{
374+
{debug_log_fn}('[{component_async_state_class}#()nextTaskExecutionSlot] next task queue length > 1000 even after cleanup, tasks may be leaking');
375+
}}
376+
}}
377+
break;
378+
}}
379+
380+
// If we get here, this task was *not* next in the queue, continue waiting
381+
// (at this point the task that *is* next will likely have already set itself
382+
// as this.#nextTaskPromise)
383+
}}
384+
}}
385+
340386
#getSuspendedTaskMeta(taskID) {{
341387
return this.#suspendedTasksByTaskID.get(taskID);
342388
}}

‎crates/js-component-bindgen/src/intrinsics/mod.rs‎

Lines changed: 65 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -180,32 +180,48 @@ impl Intrinsic {
180180
Intrinsic::Host(i) => i.render(output, args),
181181

182182
Intrinsic::GlobalAsyncDeterminism => {
183-
output.push_str(&format!(
184-
"const {var_name} = '{determinism}';\n",
183+
uwriteln!(
184+
output,
185+
"const {var_name} = '{determinism}';",
185186
var_name = self.name(),
186187
determinism = args.determinism,
187-
));
188+
);
188189
}
189190

190191
Intrinsic::CoinFlip => {
191-
output.push_str(&format!(
192-
"const {var_name} = () => {{ return Math.random() > 0.5; }};\n",
192+
uwriteln!(
193+
output,
194+
"const {var_name} = () => {{ return Math.random() > 0.5; }};",
193195
var_name = self.name(),
194-
));
196+
);
195197
}
196198

197199
Intrinsic::ConstantI32Min => output.push_str(&format!(
198200
"const {const_name} = -2_147_483_648;\n",
199201
const_name = self.name()
200202
)),
201-
Intrinsic::ConstantI32Max => output.push_str(&format!(
202-
"const {const_name} = 2_147_483_647;\n",
203-
const_name = self.name()
204-
)),
203+
204+
Intrinsic::ConstantI32Max => {
205+
uwriteln!(
206+
output,
207+
r#"
208+
const {const_name} = 2_147_483_647;
209+
"#,
210+
const_name = self.name()
211+
)
212+
}
213+
205214
Intrinsic::TypeCheckValidI32 => {
206215
let i32_const_min = Intrinsic::ConstantI32Min.name();
207216
let i32_const_max = Intrinsic::ConstantI32Max.name();
208-
output.push_str(&format!("const {fn_name} = (n) => typeof n === 'number' && n >= {i32_const_min} && n <= {i32_const_max};\n", fn_name = self.name()))
217+
218+
uwriteln!(
219+
output,
220+
r#"
221+
const {fn_name} = (n) => typeof n === 'number' && n >= {i32_const_min} && n <= {i32_const_max};
222+
"#,
223+
fn_name = self.name()
224+
);
209225
}
210226

211227
Intrinsic::AsyncFunctionCtor => {
@@ -231,25 +247,39 @@ impl Intrinsic {
231247

232248
Intrinsic::Base64Compile => {
233249
if !args.no_nodejs_compat {
234-
output.push_str("
235-
const base64Compile = str => WebAssembly.compile(typeof Buffer !== 'undefined' ? Buffer.from(str, 'base64') : Uint8Array.from(atob(str), b => b.charCodeAt(0)));
236-
")
250+
uwriteln!(
251+
output,
252+
r#"
253+
const base64Compile = str => WebAssembly.compile(
254+
typeof Buffer !== 'undefined'
255+
? Buffer.from(str, 'base64')
256+
: Uint8Array.from(atob(str), b => b.charCodeAt(0))
257+
);
258+
"#
259+
);
237260
} else {
238-
output.push_str("
239-
const base64Compile = str => WebAssembly.compile(Uint8Array.from(atob(str), b => b.charCodeAt(0)));
240-
")
261+
uwriteln!(
262+
output,
263+
r#"
264+
const base64Compile = str => WebAssembly.compile(Uint8Array.from(atob(str), b => b.charCodeAt(0)));
265+
"#
266+
);
241267
}
242268
}
243269

244-
Intrinsic::ClampGuest => output.push_str(
245-
"
246-
function clampGuest(i, min, max) {
247-
if (i < min || i > max) \
248-
throw new TypeError(`must be between ${min} and ${max}`);
249-
return i;
250-
}
251-
",
252-
),
270+
Intrinsic::ClampGuest => {
271+
uwriteln!(
272+
output,
273+
r#"
274+
function clampGuest(i, min, max) {{
275+
if (i < min || i > max) {{
276+
throw new TypeError(`must be between ${{min}} and ${{max}}`);
277+
}}
278+
return i;
279+
}}
280+
"#
281+
);
282+
}
253283

254284
Intrinsic::ComponentError => output.push_str(
255285
"
@@ -847,21 +877,25 @@ impl Intrinsic {
847877
Self::GetGlobalCurrentTaskMetaFn => {
848878
let get_current_global_task_meta_fn = Self::GetGlobalCurrentTaskMetaFn.name();
849879
let global_current_task_meta_obj = Self::GlobalCurrentTaskMeta.name();
850-
output.push_str(&format!(
880+
881+
uwriteln!(
882+
output,
851883
r#"
852884
function {get_current_global_task_meta_fn}(componentIdx) {{
853885
const v = {global_current_task_meta_obj}[componentIdx];
854886
if (v === undefined) {{ return v; }}
855887
return {{ ...v }};
856888
}}
857889
"#,
858-
));
890+
);
859891
}
860892

861893
Self::SetGlobalCurrentTaskMetaFn => {
862-
let set_global_current_task_meta_fn = Self::SetGlobalCurrentTaskMetaFn.name();
894+
let set_global_current_task_meta_fn = self.name();
863895
let global_current_task_meta_obj = Self::GlobalCurrentTaskMeta.name();
864-
output.push_str(&format!(
896+
897+
uwriteln!(
898+
output,
865899
r#"
866900
function {set_global_current_task_meta_fn}(args) {{
867901
if (!args) {{ throw new TypeError('args missing'); }}
@@ -871,7 +905,7 @@ impl Intrinsic {
871905
return {global_current_task_meta_obj}[componentIdx] = {{ taskID, componentIdx }};
872906
}}
873907
"#,
874-
));
908+
);
875909
}
876910

877911
Self::WithGlobalCurrentTaskMetaFn => {

0 commit comments

Comments
 (0)