spiderfire icon indicating copy to clipboard operation
spiderfire copied to clipboard

Race condition in the event loop?

Open Arshia001 opened this issue 1 year ago • 8 comments

This is an odd one, so I'll try my best to explain what I know.

When returning resolved promises from native code, sometimes the promise never actually resolves on the JS side. This happens right now in my implementation of TransformStream, here (but I've seen it happen elsewhere as well, unless I'm mistaken and the issue was something else there): https://github.com/wasmerio/spiderfire/blob/c87d647fcd27da7a962a0310f6d8ee5dc62cdbd4/runtime/src/globals/streams/transform_stream.rs#L246

Basically, on line 274, I'm constructing a resolved promise via this little convenience method:

pub fn new_resolved<'cx>(cx: &'cx Context, value: impl IntoValue<'cx>) -> Promise {
	let mut val = Value::undefined(cx);
	Box::new(value).into_value(cx, &mut val);

	Promise {
		promise: TracedHeap::from_local(&cx.root_object(unsafe { CallOriginalPromiseResolve(cx.as_ptr(), val.handle().into()) })),
	}
}

When I inspect the promise state on the native side, it is reported as being fulfilled. However, when returning this to the JS side, it never actually resolves (as in, the await xxx line never finished running). I tried adding reactions to the promise, and those aren't being run either. Please note I've tried using Promise::new and Promise::resolve instead of my new_resolved method and the result was the same.

Now, here's the interesting part: when I run the same code with a debugger, everything suddenly starts to work. It's the same if I add an await sleep(1000) between the construction of the promise and the await line. Both of these make me think this may be a race condition (although I don't know how, since the code is single-threaded) or otherwise has something to do with execution order or time. Unfortunately, I'm not very familiar with the event loop, so I haven't been able to investigate that area further.

Arshia001 avatar Dec 11 '23 14:12 Arshia001

More context: changing the JS code so that instead of running Promise::new_resolved on line 274, we run Promise::from on line 276, the code works without issues. The problem seems to specifically be with promises constructed on the Rust side.

Arshia001 avatar Dec 11 '23 14:12 Arshia001

~~The same error happens if I replace Promise::new_resolved with future_to_promise.~~

EDIT: using future_to_promise actually solves the issue locally. Still, since Spidermonkey itself is probably using CallOriginalPromiseResolve, that does not solve the issue completely.

Arshia001 avatar Dec 11 '23 14:12 Arshia001

More context: it actually has nothing to do with the debugger. It runs successfully under debug configuration, but fails under release.

Arshia001 avatar Dec 12 '23 08:12 Arshia001

I worked around this issue by switching to opt level 1 for WinterJS, which is the highest opt level where the error doesn't happen. I also tried staring intensely at the event loop code, but didn't find anything wrong with it. Gonna need your help with this one @Redfire75369.

Arshia001 avatar Dec 12 '23 13:12 Arshia001

@Arshia001 unfortunately redfire does not have access to laptop for the two weeks or so.

sagudev avatar Dec 12 '23 13:12 sagudev

Ah, thanks for the context. I was getting kind of worried since he's usually very responsive. I've investigated this issue as far as I know how, so I'll just wait for him to get back!

Arshia001 avatar Dec 12 '23 13:12 Arshia001

I can't seem to replicate the issue. While Promise.resolve and Promise::new -> Promise::resolve both don't trigger any event loop jobs, they should still trigger a job when they are awaited or .then'd. It's possible that this is an issue with the macrotask implementation which currently runs to completion instead of running one at a time, but I would need more context about where and how this is run for that.

Redfire75369 avatar Dec 24 '23 12:12 Redfire75369

Thanks for looking into this, I'll try to create a repro and post it.

Arshia001 avatar Dec 25 '23 10:12 Arshia001